Masterbar: Add menu structure

[obenland]

This should be functionally ready for testing.

Changes proposed in this Pull Request:

• Adds menu structure for unified admin menu.
• Adds unit tests for each new menu item and some utility methods

Jetpack product discussion

pbAPfg-VL-p2#comment-1904

Does this pull request change what data or activity we track or use?

No

Testing instructions:

To test locally:

• Add a mu-plugin that contains: add_filter( 'jetpack_load_admin_menu_class', '__return_true' );
• Make sure your site is connected to wp.com and has the masterbar module activated.
• Load wp-admin and check the sidebar!

To test with an Atomic site:

• Install & activate the Jetpack Beta Plugin on your Atomic test site.
• Go to Jetpack > Jetpack Beta
• Under "Search for Jetpack Feature Branch" enter this branch or PR id: 17629 => Click Activate
• Add a mu-plugin that contains: add_filter( 'jetpack_load_admin_menu_class', '__return_true' );
• Look at the admin menu and admire the new Menu structure.
• Click on a menu item that takes you to Calypso and marvel at this new achievement.

Proposed changelog entry for your changes:

• Not sure yet.

[jetpackbot]

	Warnings
⚠️	pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 9f6cc03

[cpapazoglou]

@obenland it seems that files and modifications are unrelated to masterbar. Do you have something in mind?

[cpapazoglou]

Thanks @obenland for working on this! Following your great test instructions 👏 I was able to admire 🎉 the new unified menu structure which contains the WordPress.com link for eg All new posts.

Using apply_filters( 'jetpack_load_admin_menu_class', ( defined( 'IS_WPCOM' ) && IS_WPCOM ) || jetpack_is_atomic_site() ) as condition seems enough as we can add_filter in wpcom and wpcomsh to enable / disable this feature.

I am not sure of the exact scope of this PR, so I will post below all my findings and we can decide how we can handle them:

1. We have mixed the masterbar and the menu. Has it been done intentionally? I suppose we should place the masterbar changes here only and create a separate module(?) for the menu?

2. We haven't ported the masterbar nav-unification changes which can be found by searching nav-unification in the trunk. I suppose this has been done intentionally as we will need to consolidate the changes with modules/masterbar/masterbar.php and modules/masterbar/overrides.css and coordinate with the calypsoify removal modules/calypsoify/class.jetpack-calypsoify.php so that we avoid

3. Moving to the menu structure itself:

Atomic	Simple

The menu items order are identical to a simple site. Adding plugins (Contact Form and Yoast) correctly shows these plugins in the menu. I suppose that ordering doesn't matter as long as it is consistent through calypso / wp-admin.

a) We need to port the new menu styles introduced here D52281-code
b) Site Switcher "Browse Sites" is missing. We need an atomic equivalent check

if ( ! is_multisite() || ( function_exists( 'get_blog_count_for_user' ) && get_blog_count_for_user() < 2 ) ) {
			return;
		}

c) Site card Default Icon needs to be updated
d) My Home "Home" submenu should link to WordPress.com
e) Stats are missing the graph image
f) Jetpack menu will need some auditing. Backup and Scan leads to wordpress.com though jetpack and it is shown as external link. Backup and Scan independent links seem not needed in this context.

g) AMP should be a single link redirecting to /wp-admin/admin.php?page=amp-options (we omit the analytics link which leads to the same page different section)

I would be happy opening separate issues for points 2 & 3 though. Let's address point 1 here!

[obenland]

Thanks for the review!

We have mixed the masterbar and the menu. Has it been done intentionally? I suppose we should place the masterbar changes here only and create a separate module(?) for the menu?

Yes, this is where we arrived in pbAPfg-VL-p2#comment-1966

We haven't ported the masterbar nav-unification changes which can be found by searching nav-unification in the trunk.

I'm not sure what you're referring to here, is there something you could link me to for context?

Site Switcher "Browse Sites" is missing. We need an atomic equivalent check

Yeah, not sure about the best way to do that one yet. Maybe wpcomsh can provide some inspiration.

AMP should be a single link redirecting to /wp-admin/admin.php?page=amp-options

Hm, I have to look into that one a bit more. Instinctively I'd probably just leave it alone since it's a separate plugin on Atomic, but I have not double-checked that yet.

Stats are missing the graph image

These seem to be only available on Simple sites.

[cpapazoglou]

Thanks for pushing the fixes @obenland !

We haven't ported the masterbar nav-unification changes which can be found by searching nav-unification in the trunk.

I'm not sure what you're referring to here, is there something you could link me to for context?

Search for nav-unification in :
/admin-bar/class.wpcom-admin-bar.php we add a .is-nav-unification in #wpadminbar
admin-bar/wpcom-admin-bar.css we use the class above to style the masterbar

eg

WithoutStyles	WithStyles

Stats are missing the graph image

These seem to be only available on Simple sites.

True!

Fix site card icon

Sadly this was not enough. It seems that the following wrongly returns true and therefore styles are not applied correctly.

if ( ( function_exists( 'blavatar_exists' ) && blavatar_exists( $domain ) ) || has_site_icon() ) {
  $classes .= ' has-site-icon';
}

Wrong	Correct

I can confirm that the rest of the changes are LGTM!

[obenland]

@cpapazoglou 867a910 should fix the site icon issue on WoA sites.

[cpapazoglou]

@cpapazoglou 867a910 should fix the site icon issue on WoA sites.

Indeed, I can confirm that is fixed! Now, we just need the masterbar missing styles.

[getdave]

FYI @obenland this PR went through a number of iterations to create a structure Jetpack crew are happy with. It might be best to wait till that's merged and then put your new changes on top of that.

#17762

[obenland]

Admin Bar mobile styles

WithoutStyles	WithStyles

To make this work on WoA sites, we'll have to add the .is-nav-unification class to the body element since WP_Admin_Bar doesn't have a filter for that. It also looks like the the menu items are not 1:1 the same between simple sites and WoA. So even with the custom CSS class manually added to the admin bar element, the menu toggle doesn't show up on small screens.

[cpapazoglou]

Admin Bar mobile styles

WithoutStyles WithStyles

To make this work on WoA sites, we'll have to add the .is-nav-unification class to the body element since WP_Admin_Bar doesn't have a filter for that. It also looks like the the menu items are not 1:1 the same between simple sites and WoA. So even with the custom CSS class manually added to the admin bar element, the menu toggle doesn't show up on small screens.

Actually we don't need .is-nav-unification.. We can leverage add_filter( 'jetpack_load_admin_menu_class', '__return_true' ); or create a new filter which will load an extra .css file.

The weird thing is that jetpack already loads wpcom styles & wpcom overrides

jetpack/modules/masterbar/masterbar.php

Lines 267 to 268 in 8ab0362

	wp_enqueue_style( 'a8c-wpcom-masterbar', $this->wpcom_static_url( '/wp-content/mu-plugins/admin-bar/wpcom-admin-bar.css' ), array(), JETPACK__VERSION );
	wp_enqueue_style( 'a8c-wpcom-masterbar-overrides', $this->wpcom_static_url( '/wp-content/mu-plugins/admin-bar/masterbar-overrides/masterbar.css' ), array(), JETPACK__VERSION );

So, I suggest:

• temporarily include the changes in

  jetpack/modules/masterbar/admin-menu.css

  Line 1 in 4d35e20

  #adminmenu {

  as it is conditionally loaded by the filter.
• discard the .is-nav-unification, as filter alone is enough
• when we launch to 100% and remove the filter, also move the styles to

  jetpack/modules/masterbar/overrides.css

  Line 1 in d6211b1

  /* Remove min-height from menu elements that was causing them to render incorrectly */

  
  or better to wpcom /wp-content/mu-plugins/admin-bar/wpcom-admin-bar.css

Last but not least, I can confirm that without using add_filter( 'jetpack_load_admin_menu_class', '__return_true' ); the whole feature is disabled and when I used add_filter( 'jetpack_load_admin_menu_class', '__return_true' ); the feature was enabled as expected.

[obenland]

@anomiex Thanks for your review! I fixed up the things you pointed out, this one should be good for another round of reviews.

[cpapazoglou]

Fixed the menu problem here #17905

[obenland]

@jeherve @anomiex I hope the stress of releasing 9.2 has died down a bit. Any chance you could give this a final ✅ at your convenience?

[jeherve]

Still need to test this some more, but I had a few questions.

[obenland]

Still need to test this some more, but I had a few questions.

Thank you for your feedback! Happy to make any amendments as you see fit. I also expect there to be quite a few more follow up PRs to this, particularly once we opened it to a8c-internal testing this week.

[jeherve]

I have some additional remarks, but I'm approving this PR anyway, for a couple of reasons:

• I believe we could really benefit from some additional testing in different scenarios, with popular plugins like WooCommerce, WooCommerce extensions, Yoast SEO, Elementor, et al., as well as popular themes that extend the Themes menu. By adding this to Jetpack now, it should make it easier for folks to test on Atomic. I suspect we'll find quite a few issues with such plugins and how they interact with the existing admin menu.
• The feature is still gated by a filter, so not breaking anything by default.
• That PR is getting big, and the more we wait, the harder it will be to review.

[jeherve]

Is it problematic that folks will lose access to this option? I know we discourage its use in general, but it remains available in wp-admin anyway, even on Atomic. I wonder if this would cause some issues for folks in support in some cases.

[obenland]

@davemart-in Do you have thoughts on the removal/non-removal of theme & plugin editor links?

[davemart-in]

Let me phone a few friends. @kristenzuck, @nerdysandy thoughts on removing links to the theme & plugin editor from a happiness perspective? These pages would still be available if you knew how to get to them, but I've proposed removing them from the default menu. I'm happy to change my position on this one. Just curious if you see these pages used by many customers.

[jeherve]

Just like above, is it problematic to remove the plugin editor?