Notifications: do not rely on admin-bar styles

[jeherve]

Follow up from #13450

Changes proposed in this Pull Request:

• In some cases, admin-bar styles may be dequeued (like when the Masterbar module is active). Let's consequently not rely on them to be there.

cc @westonruter

Testing instructions:

• Before to apply this patch, enable both Notifications and the Masterbar module.
• Notice the broken layout.
• Apply patch.
• Notice that both the Masterbar and the Notifications work well.

Proposed changelog entry for your changes:

• None

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: November 5, 2019.
Scheduled code freeze: October 29, 2019

Generated by 🚫 dangerJS against ed1f3e6

[westonruter]

@jeherve If the stylesheet is not marked as having an admin-bar dependency, then you'll need to manually mark it as being part of AMP dev mode, as can be seen with the script_loader_tag filter for scripts. So something like this:

if ( class_exists( 'Jetpack_AMP_Support' ) && Jetpack_AMP_Support::is_amp_request() ) {
	add_filter(
		'style_loader_tag',
		function ( $tag, $handle ) {
			if ( in_array( $handle, [ 'wpcom-notes-admin-bar', 'noticons' ], true ) ) {
				$tag = preg_replace( '/(?<=<link)(?=\s|>)/i', ' data-ampdevmode', $tag );
			}
			return $tag;
		},
		10,
		2
	);
}

[jeherve]

@westonruter That makes sense, thank you. How about a different approach, like this?
ed1f3e6

[westonruter]

I don't know much about the Masterbar, but the change should work. Nevertheless, since wpcom-notes-admin-bar has admin-bar as a dependency, would wp_dequeue_style() even do anything since it isn't being directly enqueued to begin with?

[kraftbj]

I'm marking this as approved since it does resolve the original issue.

When using AMP in canonical mode, the traditional admin bar is on the front end -- is this expected? No issues or errors, but different behavior with AMP on vs off.

Marking as Need Author Reply as I'm deferring to @jeherve's opinion, but fine to merge to me to help Atomic and we can fix up if the AMP issue is an issue later.

[kristarella]

When using AMP in canonical mode, the traditional admin bar is on the front end -- is this expected? No issues or errors, but different behavior with AMP on vs off.

I noticed this when testing AMP on the weekend. Does Jetpack influence this? There used to be an option in the AMP plugin whether or not to display the admin bar, but that option is no longer there. IMO it would be better not to display the admin bar because it is adding extra CSS to the page that might affect the CSS treeshake (like bumping out other CSS for being over the 50kb limit).

[westonruter]

As of AMP v1.3 the admin bar no longer counts against the 50KB limit. This is why there is no longer an option to disable the admin bar on AMP pages.

[kraftbj]

To clarify, my curiosity was more that the traditional admin bar was present on an AMP page and not the wpcom masterbar. Between the options of the traditional admin bar or no admin bar at all, I'd like to see the traditional admin bar given AMP canonical would be the primary post view experience.

[jeherve]

since wpcom-notes-admin-bar has admin-bar as a dependency, would wp_dequeue_style() even do anything since it isn't being directly enqueued to begin with?

It does, yes. In practice, you end up with the admin bar not being loaded at all:

When using AMP in canonical mode, the traditional admin bar is on the front end -- is this expected

I'd say yes. As you point out the traditional admin bar seems like a better fit there.