Skip to content
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

Restore admin bar in AMP responses #1205

Closed
westonruter opened this issue Jun 7, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@westonruter
Copy link
Member

commented Jun 7, 2018

In 0.7 we disabled the admin bar on the frontend when in AMP because of the additional CSS and JS it adds to the page that would cause it to be invalid:

https://github.com/Automattic/amp-wp/blob/6e6275b84bba131010a90dbea4d325986728057b/includes/class-amp-theme-support.php#L330-L334

However, now with tree-shaking this is less of a concern. In fact, I think we should restore showing the admin bar in AMP responses, but provide an additional checkbox on the general settings screen for whether to show the admin bar in AMP. This could be under Validation Handling since the admin bar could be a source for validation errors.

The following patch gets the admin bar working in AMP:

--- a/includes/class-amp-theme-support.php
+++ b/includes/class-amp-theme-support.php
@@ -350,11 +350,22 @@ class AMP_Theme_Support {
 
 		add_action( 'wp_footer', 'amp_print_analytics' );
 
-		/*
-		 * Disable admin bar because admin-bar.css (28K) and Dashicons (48K) alone
-		 * combine to surpass the 50K limit imposed for the amp-custom style.
-		 */
-		add_filter( 'show_admin_bar', '__return_false', 100 );
+		add_action( 'admin_bar_init', function() {
+			wp_dequeue_script( 'admin-bar' );
+			wp_add_inline_style( 'admin-bar', '#wpadminbar.nojs { display:block; } #wpadminbar.nojs li:focus-within > .ab-sub-wrapper { display:block; }' );
+			add_filter( 'body_class', function( $body_classes ) {
+				return array_merge(
+					array_diff(
+						$body_classes,
+						array( 'no-customize-support' )
+					),
+					array( 'customize-support' )
+				);
+			} );
+			add_action( 'admin_bar_menu', function() {
+				remove_action( 'wp_before_admin_bar_render', 'wp_customize_support_script' );
+			}, 41 );
+		} );
 
 		/*
 		 * Start output buffering at very low priority for sake of plugins and themes that use template_redirect

@westonruter westonruter added this to the v1.0 milestone Jun 7, 2018

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2018

There is one known issue with the admin bar that is not addressed by the above patch. The profile pic gets rendered incorrectly probably due to some * universal selectors:

image

image

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2018

Also, instead of adding the #wpadminbar.nojs { display:block; } style, we should just do the following at the admin_bar_init action:

remove_action( 'wp_head', 'wp_admin_bar_header' );
@amedina

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

Would it be possible to provide a toggle that allows users to dynamically add/remove the add_filter( 'show_admin_bar', '__return_false', 100 ); ?

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jun 10, 2018

Would it be possible to provide a toggle that allows users to dynamically add/remove

Yes, that's exactly what I'm thinking, re: “provide an additional checkbox on the general settings screen for whether to show the admin bar in AMP”.

@ktmn

This comment has been minimized.

Copy link

commented Jul 28, 2018

@westonruter Can the admin bar also be enabled for classic mode? I played around with 1.0 and couldn't get it to show.

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2018

@ktmn It's possible but it would take quite a bit of work to figure out. Since classic mode will be deprecated on favor of using theme support going forward, I don't think it makes sense to expend the effort to introduce new features for it.

@ktmn

This comment has been minimized.

Copy link

commented Jul 29, 2018

@westonruter Oh classic is being deprecated, as in completely removed in the future?

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

@ktmn It won't be removed, but we don't have plans to further improve it to a significant degree. So the templates are going to remain as-is, e.g. no addition of nav menus and other features. So it will be “soft-deprecated” as in discouraged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.