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

Introduce admin bar integration on AMP pages #438

Closed
westonruter opened this issue Aug 24, 2019 · 9 comments
Closed

Introduce admin bar integration on AMP pages #438

westonruter opened this issue Aug 24, 2019 · 9 comments
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Milestone

Comments

@westonruter
Copy link
Collaborator

westonruter commented Aug 24, 2019

Feature Description

Currently the full admin bar integration is disabled on AMP pages:

$admin_bar_callback = function() {
if ( ! $this->is_active() ) {
return;
}
// Enqueue fonts.
$this->assets->enqueue_fonts();
// Enqueue styles.
$this->assets->enqueue_asset( 'googlesitekit_adminbar_css' );
if ( $this->is_amp() ) {
return;
}
// Enqueue scripts.
$this->assets->enqueue_asset( 'googlesitekit_adminbar_loader' );
};

However, thanks to a new AMP “dev mode” capability, there is now a proper way to include invalid AMP markup on an otherwise valid AMP page. When the html element has a data-ampdevmode attribute, then any element which also has this same attribute will have its validation errors suppressed. See ampproject/amphtml#20974 and ampproject/amp-wp#3084. In dev mode, the AMP validator extension will not complain about validation errors: ampproject/amphtml#24176.

This is exactly what the AMP plugin has needed to prevent the admin bar from getting excluded due to the additional CSS that the admin bar requires. For details on that, see ampproject/amp-wp#1921.

This opens up opportunities for Site Kit as well. As long as the googlesitekit_adminbar_loader script and its dependencies get the data-ampdevmode attribute added to it, then they will not get removed by the sanitizer and the admin bar on AMP pages can be just as interactive as on non-AMP pages.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • When AMP dev mode is enabled (via AMP plugin >= 1.4's amp_is_dev_mode()), the admin bar menu of Site Kit should expand in the frontend for an AMP request just like it does for a non-AMP request. Clicking on the menu should not take you to the admin like it does in AMP without dev mode.

Implementation Brief

Changelog entry

  • Add support for displaying the full Site Kit admin bar menu with stats also for AMP requests, by leveraging AMP's dev mode feature.
@jamesozzie jamesozzie added the Type: Enhancement Improvement of an existing feature label Aug 26, 2019
@felixarntz felixarntz added P1 Medium priority Size: XS labels Oct 27, 2019
@lilybonney lilybonney added this to the 1.0.2 milestone Nov 4, 2019
@felixarntz felixarntz modified the milestones: 1.0.2, 1.1.0 Nov 4, 2019
@lilybonney lilybonney removed this from the 1.1.0 milestone Nov 4, 2019
@felixarntz felixarntz added this to the 1.1.0 milestone Nov 20, 2019
@tofumatt tofumatt self-assigned this Nov 20, 2019
@tofumatt
Copy link
Collaborator

What are the steps to QA this issue/reproduce the results? I'm not super-familiar with AMP or the AMP plugin, so I've tried installing the latest AMP plugin and using a browser extension to load AMP pages, but I'm unclear as to what to check here. I see the admin bar in pages when I enable "Standard" AMP across the site.

I see the Site Kit section in the WordPress admin bar on what claim to be "AMP pages" in the latest develop. If that's what this issue is referring to then it seems to be working, but I'm not clear on that/confident the AMP output is working as expected given it doesn't look any different in my browser.

So I think this is right but if someone could verify my interpretation of this issue that'd be appreciated. 🙂

@tofumatt tofumatt removed their assignment Nov 20, 2019
@westonruter
Copy link
Collaborator Author

If you activate the AMP plugin and in the AMP plugin settings switch from “Reader” to “Standard” mode, you should then be able to the Site Kit functionality in the admin bar just the same as when AMP is turned off. Previously in AMP the Site Kit menu item would not show the details for the current URL when tapping.

In addition to the admin bar being fully functional in AMP pages, the AMP plugin should likewise not be reporting any validation errors from Site Kit. In other words, you should see a ✅ in the AMP admin bar menu item.

@tofumatt tofumatt self-assigned this Nov 21, 2019
@tofumatt
Copy link
Collaborator

Hmmm, I don't see a checkmark in the admin bar, but I don't see any errors on the actual AMP details page:

2019-11-21 12 15 27

I'm not sure if something else is off here or if that's a bug in this patch.

@tofumatt
Copy link
Collaborator

(I should be clear: this is a local site running on my Mac—if AMP is expecting to be able to request these URLs from a Google server somewhere I'd expect them to fail.)

@felixarntz
Copy link
Member

@tofumatt It shouldn't have problems because of a local site, there may be issues though if your local setup doesn't allow your server to run requests against itself. Alternatively, does this site have any potentially incompatible plugins?

@tofumatt
Copy link
Collaborator

No other plugins are enabled, just AMP 1.4.1 and the latest Site Kit from develop.

Oddly I get this error when activating AMP:

Screenshot 2019-11-21 13 01 42

I didn't notice it before; maybe it's new.

@felixarntz
Copy link
Member

Yeah that is probably related to your server not being able to run a request to itself.

@tofumatt
Copy link
Collaborator

Ah, okay, looks like something was off with my local setup:

Screenshot 2019-11-21 at 13 34 33

QA ✅

@tofumatt tofumatt removed their assignment Nov 21, 2019
@marrrmarrr
Copy link
Collaborator

LGTM.

@ThierryA ThierryA modified the milestones: 1.1.0, Sprint 11 Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants