New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update/Settings ui #9952

Merged

enejb merged 19 commits into master from update/publicize-settings-ui

Sep 17, 2018

Member

enejb commented Jul 26, 2018 •

edited

Loading

Adds the the same wrapper of the admin to the shareing settings.

Before:
Stats Screen

Shareing Settings Screen

Debug screen in wp-admin

After:

Changes proposed in this Pull Request:

Testing instructions:

Check out the different wp-admin pages now.
Click around. Resize the window. Try in it on different devices.

Jetpack > Stats
Jetpack > Stats > Settings
Jetpack > Bulk settings > wp-admin/admin.php?page=jetpack_modules
Settings > Sharing
Jetpack (footer) Debug link
Network Admin (need to create a MULTI SITE FOR THIS) PROTIP: (Use https://jurassic.ninja/specialops/ )
-- Network > Jetpack > Sites
-- Network > Jetpack > Settings

Proposed changelog entry for your changes:

enejb requested review from joanrho, zinigor and dereksmart

July 26, 2018 04:23

enejb requested a review from a team as a code owner

July 26, 2018 04:23

matticbot added the Touches WP.com Files label

Contributor

matticbot commented Jul 26, 2018

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D16513-code. (newly created revision)

enejb added [Status] Needs Review [Status] Needs Design Review Admin Page [Feature] Sharing [Feature] Stats Data labels

enejb added this to the 6.4 milestone

enejb requested a review from kraftbj as a code owner

July 26, 2018 04:33

Contributor

matticbot commented Jul 26, 2018

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D16513-code. (updated diff)

1 similar comment

Contributor

matticbot commented Jul 26, 2018

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D16513-code. (updated diff)

joanrho approved these changes

View reviewed changes

Contributor

joanrho left a comment

tested all and LGTM (except the two mobile viewports we chatted about: https://cloudup.com/cIwrylYHUnP)! But we can cover that once we do a full sweep of all our admin interfaces.
Thanks @enejb!

Contributor

oskosk commented Jul 31, 2018

This one needs a rebase @enejb

enejb force-pushed the update/publicize-settings-ui branch from ea0a509 to c12775c Compare

August 1, 2018 16:30

brbrr added the [Type] Enhancement label

oskosk modified the milestones: 6.4, 6.5

Contributor

oskosk commented Aug 7, 2018

This one needs a rebase again. Sorry :(

enejb changed the title ~~Update/publicize settings ui~~ Update/Settings ui

Contributor

matticbot commented Aug 8, 2018

Caution: This PR has changes that must be merged to WordPress.com
Please review this diff before merging: D16513-code. (updated diff)

enejb force-pushed the update/publicize-settings-ui branch from c12775c to d31578c Compare

August 8, 2018 17:02

Contributor

brbrr commented Aug 14, 2018 •

edited

Loading

Hey @enejb,
This PR looks like contains a bug. When I opening ‘Sharing’ settings as not connected user (wp-admin > Settings > Sharing ) I see a blank page (empty, no content). No JS errors, no wp-error.log entries. (it seems like WPOD)

more details (internal ref): p8oabR-fW-p2

brbrr removed the [Status] Needs Review label

Member Author

enejb commented Aug 17, 2018

I am not sure how you are testing this but I see the following

Contributor

brbrr commented Aug 23, 2018

Sorry. I mixed up PRs, This PR looks good

brbrr reviewed

View reviewed changes

Contributor

brbrr left a comment

Functionally tested. LGTM.

I'd like to ask @zinigor / @mdawaffe to check the code changes since they are quite big.

modules/stats.php Outdated

@@ @@ -715,6 +686,7 @@ function stats_reports_page( $main_chart_only = false ) { @@
               				die();
               			}
               		}
+              		error_log( print_r( $get, 1) );

Contributor

brbrr Aug 23, 2018

Is this debug statement needed here?

jeherve assigned enejb

Member Author

enejb commented Sep 11, 2018

Can you explain a bit how the fatal might happen here? I don't quite understand it.

The way I understand it is the file would never be loaded as far as I can tell. Since there is nothing that would try to load it. With the debug module file. that is not the case since the module would try to be loaded from options. Since that value might be stored in the option.

enejb removed the [Status] Needs Author Reply label

enejb dismissed jeherve’s stale review

September 11, 2018 17:41

answer the question.

Member

jeherve commented Sep 12, 2018

Can you explain a bit how the fatal might happen here? I don't quite understand it.

I unfortunately was never able to reproduce the issue, but we've had it happen on a few updates when removing different files. From our tests when that happened, it often seemed like a caching issue.

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php

+              		$rtl = is_rtl() ? '.rtl' : '';
+              		wp_enqueue_style( 'dops-css', plugins_url( "_inc/build/admin.dops-style{$rtl}.css", JETPACK__PLUGIN_FILE ), array(), JETPACK__VERSION );
+              		wp_enqueue_style( 'components-css', plugins_url( "_inc/build/style.min{$rtl}.css", JETPACK__PLUGIN_FILE ), array(), JETPACK__VERSION );
+              		$custom_css = '

Contributor

eliorivero Sep 13, 2018

Is there a particular reason for this CSS to be written here? If possible, it should be moved to a stylesheet.

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php Outdated

+              							<span class="dops-button-group">
+              								<?php
+              								if ( current_user_can( 'jetpack_network_sites_page' ) ) {
+              									?><a href="<?php echo esc_url( network_admin_url( 'admin.php?page=jetpack' ) ); ?>" type="button" class="<?php echo esc_attr( $highlight_current_sites ); ?> dops-button is-compact" title="<?php esc_html_e( 'Manage your network\'s Jetpack Sites.', 'jetpack' ); ?>"><?php echo esc_html_x( 'Sites', 'Navigation item', 'jetpack' ); ?></a><?php

Contributor

eliorivero Sep 13, 2018

To make it easier for translators, this string should be within double quotes so there's no need to use a backslash:

"Manage your network's Jetpack Sites."

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php Outdated

+              								if ( current_user_can( 'jetpack_network_sites_page' ) ) {
+              									?><a href="<?php echo esc_url( network_admin_url( 'admin.php?page=jetpack' ) ); ?>" type="button" class="<?php echo esc_attr( $highlight_current_sites ); ?> dops-button is-compact" title="<?php esc_html_e( 'Manage your network\'s Jetpack Sites.', 'jetpack' ); ?>"><?php echo esc_html_x( 'Sites', 'Navigation item', 'jetpack' ); ?></a><?php
+              								} if ( current_user_can( 'jetpack_network_settings_page' ) ) {
+              									?><a href="<?php echo esc_url( network_admin_url( 'admin.php?page=jetpack-settings' ) ); ?>" type="button" class="<?php echo esc_attr( $highlight_current_settings ); ?> dops-button is-compact" title="<?php esc_html_e( 'Manage your network\'s Jetpack Settings.', 'jetpack' ); ?>"><?php echo esc_html_x( 'Network Settings', 'Navigation item', 'jetpack' ); ?></a><?php

Contributor

eliorivero Sep 13, 2018

Same here, using double quotes would help translation.

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php Outdated

+              						<a href="https://wordpress.com/tos/" target="_blank" rel="noopener noreferrer" title="<?php esc_html__( 'WordPress.com Terms of Service', 'jetpack' ); ?>" class="jp-footer__link"><?php echo esc_html_x( 'Terms', 'Navigation item', 'jetpack' ); ?></a>
+              					</li>
+              					<li class="jp-footer__link-item">
+              						<a href="<?php echo esc_url( $jetpack_admin_url . '#/privacy' ); ?>" rel="noopener noreferrer" title="<?php esc_html_e( 'Automattic\'s Privacy Policy', 'jetpack' ); ?>" class="jp-footer__link"><?php echo esc_html_x( 'Privacy', 'Navigation item', 'jetpack' ); ?></a>

Contributor

eliorivero Sep 13, 2018 •

edited

Loading

Use double quotes in translatable string to avoid backslash.

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php Outdated

+              					</li>
+              					<?php if ( is_multisite() && current_user_can( 'jetpack_network_sites_page' ) ) { ?>
+              						<li class="jp-footer__link-item">
+              							<a href="<?php echo esc_url( network_admin_url( 'admin.php?page=jetpack' ) ); ?>" title="<?php esc_html_e( 'Manage your network\'s Jetpack Sites.', 'jetpack' ); ?>" class="jp-footer__link"><?php echo esc_html_x( 'Network Sites', 'Navigation item', 'jetpack' ); ?></a>

Contributor

eliorivero Sep 13, 2018 •

edited

Loading

Use double quotes in translatable string to avoid backslash.

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php Outdated

+              					<?php } ?>
+              					<?php if ( is_multisite() && current_user_can( 'jetpack_network_settings_page' ) ) { ?>
+              						<li class="jp-footer__link-item">
+              							<a href="<?php echo esc_url( network_admin_url( 'admin.php?page=jetpack-settings' ) ); ?>" title="<?php esc_html_e( 'Manage your network\'s Jetpack Settings.', 'jetpack' ); ?>" class="jp-footer__link"><?php echo esc_html_x( 'Network Settings', 'Navigation item', 'jetpack' ); ?></a>

Contributor

eliorivero Sep 13, 2018 •

edited

Loading

Use double quotes in translatable string to avoid backslash.

eliorivero reviewed

View reviewed changes

_inc/lib/admin-pages/class.jetpack-admin-page.php Outdated

+              					<?php } ?>
+              					<?php if ( current_user_can( 'manage_options' ) ) { ?>
+              						<li class="jp-footer__link-item">
+              							<a href="<?php echo esc_url( admin_url() . 'admin.php?page=jetpack-debugger' ); ?>" title="<?php esc_html_e( 'Test your site\'s compatibility with Jetpack.', 'jetpack' ); ?>" class="jp-footer__link"><?php echo esc_html_x( 'Debug', 'Navigation item', 'jetpack' ); ?></a>

Contributor

eliorivero Sep 13, 2018

Use double quotes in translatable string to avoid backslash.

enejb added 2 commits

September 14, 2018 10:25


          Update the translation strings so that they don't have backslashes.

6f3aebf


          Deprecate deleted files

ae05fe0

jeherve approved these changes

View reviewed changes

Member

jeherve left a comment

This looks good to me now.

jeherve added the [Status] Ready to Merge label

enejb merged commit 97a669d into master

matticbot added the [Status] Needs Changelog label

enejb deleted the update/publicize-settings-ui branch

September 17, 2018 16:10

ghost removed the [Status] Ready to Merge label

dereksmart reviewed

View reviewed changes

modules/sharedaddy/sharing.php

               <?php
               	}
+              	public function wrapper_admin_page() {
+              		Jetpack_Admin_Page::wrap_ui( array( &$this, 'management_page' ), array( 'is-wide' =>true ) );

Member

dereksmart Sep 17, 2018

Jetpack_Admin_Page::wrap_ui is only available in Jetpack - this file cannot be synced back as-is

Contributor

kraftbj Sep 20, 2018

Would it make sense to add this on the WP.com side since additional wp-admin pages in the future will likely adopt using this function?

jeherve added [Status] Needs Testing [Status] Has Changelog and removed [Status] Needs Changelog labels

jeherve added a commit that referenced this pull request


          changelog: add #9952

a4870f3

jeherve added a commit that referenced this pull request


          Changelog for 6.6 release (#10107)

581489a

* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193

jeherve mentioned this pull request

Multisite: Jetpack's network admin "sites" page is broken #11186

Closed

jeherve removed the [Status] Needs Testing label

kraftbj mentioned this pull request

Jetpack: Remove unused file #21205

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eliorivero eliorivero left review comments

brbrr brbrr left review comments

dereksmart dereksmart left review comments

joanrho joanrho approved these changes

jeherve jeherve approved these changes

zinigor Awaiting requested review from zinigor

kraftbj Awaiting requested review from kraftbj

Labels

Admin Page [Feature] Sharing [Feature] Stats Data [Status] Design Review Complete Touches WP.com Files [Type] Enhancement