-
Notifications
You must be signed in to change notification settings - Fork 846
Jetpack plugin: Require and use jetpack-masterbar package #37676
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.
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
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
| */ | ||
| public function enqueue_scripts() { | ||
| $assets_base_path = '../../dist/admin-menu/'; | ||
| if ( $this->is_rtl() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean up. This is no longer used below
...ns/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-admin-menu.php
Show resolved
Hide resolved
|
Wow @fgiannar, impressive work! Everything seems to be working as expected for me.
Probably no longer needed now that the inline help is powered by the ETK plugin:
Yeah, me neither. When I removed the plan from SA, Jetpack gets deactivated so the masterbar module does not run. I guess it's not a big deal – it probably works as expected (and if it doesn't, in practice all Atomic sites have the needed plan except for a brief time after plan expiration, so we'll only have very few edge cases). |
| remove_action( 'customize_register', array( 'Jetpack_Fonts_Typekit', 'maybe_override_for_advanced_mode' ), 20 ); | ||
|
|
||
| remove_action( 'customize_register', 'Automattic\Jetpack\Dashboard_Customizations\register_css_nudge_control' ); | ||
| remove_action( 'customize_register', 'Automattic\Jetpack\Masterbar\register_css_nudge_control' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is currently not working for Simple sites since we are still using the module code on WPCOM. We should probably add a class_exists here so that when we are on WPCOM (Simple sites) we keep using Automattic\Jetpack\Dashboard_Customizations\register_css_nudge_control, while on remote sites (self-hosted and WoA) we use Automattic\Jetpack\Masterbar\register_css_nudge_control
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done via 6aa5a1e
…oved depending on the env
...ns/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-admin-menu.php
Show resolved
Hide resolved
…ding admin-menu but more explicit
mmtr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, tried to be more thorough this time (tested with Simple Classic, Simple Default, Atomic Classic, Atomic Default, Self-hosted with toolbar module enabled, and Self-hosted with toolbar module disabled) and everything seems to be working as expected for me.
Considering the number of scenarios and features to check, it'd be great if someone else could test this as well!
coder-karen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on Atomic, self-hosted and Simple. Everything works as expected now.
RTL actually works better from my tests on Atomic - the notification panel works there but doesn't on trunk, when RTL is enabled.
bindlegirl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and everything seems to be fine.
Load the Masterbar module (WordPress.com Toolbar) functionality from the
masterbar-packageinstead of using the corresponding Module code, soon to be deprecated.Proposed changes:
jetpack\modules\masterbar.php: Use package to load the Masterbar module functionalityOther information:
Jetpack product discussion
pfwV0U-3U-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Important! While testing on WoA make sure to also activate the WordPress.com Features plugin (Bleeding Edge just in case) via Beta Tester and add
define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true );to yourwp-config.phpnav-unification: Testing the admin menu in Simple, WoA and self-hosted:
You can test this in the WPCOM REST API Dev console for the
wpcom/v2/sites/YOUR_BLOG_ID/admin-menuendpoint.For block based themes, "Customize" should not be present under "Appearance".
Make sure to test with/without the PR applied and confirm the responses are identical. For Simple and WoA sites we'll also need to test with classic/default Admin Interface Style.
nav-unification: Admin color schemes
For self-hosted sites and classic WoA sites, upon editing a User Profile via wp-admin, confirm the Admin Color Scheme field is present:
Ensure that by picking one of the options, the admin theme is updated.
For default WoA sites, upon editing a User Profile via wp-admin, confirm you see the following link to update the field on WPCOM:

For a WoA site, this change should be also reflected in Calypso.
Confirm that the
admin_coloruser meta is also part of the corresponding REST API response to theusersendpoint.You can test this in the WPCOM REST API Dev console for the
wp/v2/sites/YOUR_BLOG_ID/usersendpoint.Make sure to test this with a self-hosted and WoA. Not applicable for Simple sites.
Classic vs Default view
Try switching between Classic vs Default view for a WoA and Simple site. Confirm nothing breaks and both views are displayed as expected.

For self-hosted sites, this option should NOT be visible since there should be no nav customizations on WP Admin of Jetpack sites
For WoA sites with SSO disabled this option should NOT be visible since there should be no nav customizations on WP Admin of Atomic sites when SSO is disabled.
Editing a user profile on wp-admin for WoA sites
When editing a user's profile via wp-admin on a WoA site, confirm that you see the following message and the corresponding links to your WPCOM account page work as expected:
Inline Help
TBA
Nudges to purchase a WordPress.com Plan to be able to customize CSS on your site (Simple and WoA sites)
Make sure your site is 1) using a non-block theme, eg Hever. and 2) IS NOT on the explorer plan
Navigate to Appearance -> Additional CSS and confirm you see the following nudge:
Ensure the Upgrade flow is working as expected by clicking the "Upgrade now" button. You should be redirected to checkout with the Explorer plan in your cart.
Note sure how to test this on a WoA site
Disable edit_post and delete_post capabilities for Posts Pages in WP-Admin and display a notification icon.
See #19900
RTL support
Simple & WoA: Update your WPCOM user locale to Arab and ensure the Masterbar and Admin menu is displayed consistently with/without the PR.
Self hosted: Update your user locale to Arab and ensure the Masterbar and Admin menu is displayed consistently with/without the PR.
Let's make sure to test this as much as possible since the package loads RTL styles automatically via the Assets package.
Example: All the relevant
$this->is_rtl()conditionals in the module code have been removed from the corresponding package code.