-
Notifications
You must be signed in to change notification settings - Fork 834
Blaze: ensure the Dashboard menu is only inserted once. #31617
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
This fixes issues on WoA sites and WordPress.com Simple sites, where when the new dashboard is enabled, it is added by the package while the old Calypso link is also added in the masterbar.
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 (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
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.
I tested in simple/atomic and self-hosted sites, and the menu option always pointed to the correct place. (wpcom for simple/atomic, and new dashboard for self-hosted).
The code looks good 👍
// performance settings already have a link to Page Optimize settings page. | ||
$this->hide_submenu_page( 'options-general.php', 'page-optimize' ); | ||
|
||
if ( Blaze::should_initialize() ) { |
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.
I checked and this code is now inside the Blaze package. This means that the Advertising menu will appear only if the Blaze module is enabled. Right?
I think it is already like this right now, you are just moving this code to the blaze package.
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.
That's correct, yes.
One more question @jeherve, after we merged/deployed this change. Would it be a way to test the Blaze Dashboard on atomic sites? |
That's a very good point, and something I should have addressed in this PR. Since the menu is not registered on WordPress.com sites, you cannot and should not be able to get to it. Right now you still can, via the quick links in the posts list, or via the post-publish panel. I'll get this fixed. |
We want to be able to test the new Dashboard on WoA if we want to, by supplying the jetpack_blaze_dashboard_enable filter.
This is a follow-up to #31617 and #31750. We now have Blaze::enable_blaze_menu() to add the Blaze menu to wp-admin whenever necessary. However, Jetpack sites still rely on the admin-menu endpoint to display a list of custom menu items in Calypso. There, we'll want to always display the Tools > Advertising menu item, as long as the site is eligible for Blaze.
Proposed changes:
This fixes issues on WoA sites and WordPress.com Simple sites, where when the new dashboard is enabled, the navigation menu (under Tools > Advertising) is added by the package, while the old Calypso link is also added, via the masterbar.
To solve this problem, we rely on the package, and only the package, to register the menu. We do point WoA and simple site owners to the Calypso advertising page (and not the wp-admin advertising page) at all times, since they have nav-unification on and are logged in to WordPress. com at all times.
This PR also changes the order of the new menu added by the package, to ensure it's always added at the top of the Tools menu.
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
You'll want to test this on all three site types (WoA, Simple, self-hosted).
add_filter( 'jetpack_blaze_dashboard_enable', '__return_true' );
, you should see a new menu item, pointing you to the wp-admin advertising page.