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

Add support for AMP Dev Mode in Notes module #13450

Merged
merged 4 commits into from Oct 5, 2019

Conversation

@westonruter
Copy link
Contributor

commented Sep 8, 2019

As noted in ampproject/amp-wp#1921, AMP now supports a dev mode (ampproject/amphtml#20974, ampproject/amp-wp#3187) which allows a document to mark certain elements as being excluded for AMP validation. This is exactly what the AMP plugin has needed to allow the admin bar without fighting against the 50KB CSS limit. It also opens up the door to using scripts on AMP pages to add interactivity to the admin bar without worrying about AMP compatibility. This is exactly what Jetpack has needed for its admin bar integration on AMP pages (e.g. Stats and Notes).

This PR begins to implement support for that. Any markup that is being added by Jetpack to the frontend which is used by an authenticated user for administration should be amended with the data-ampdevmode attribute to each element. The elements in the admin bar items will get this automatically, so the primary concern is the script, link, and style elements that are output to extend on the frontend admin bar.

For more information on this, see Integrating with AMP Dev Mode in WordPress.

Part of #9730. This revisits/reverts aspects of #10945.

Support implemented in these modules:

Before:

Screen Shot 2019-10-02 at 09 02 04

Screen Shot 2019-10-02 at 09 02 24

After:

Screen Shot 2019-10-02 at 09 00 13

Screen Shot 2019-10-02 at 08 59 22

Notice the Notes admin menu item is not broken any longer after the changes are applied.

To revisit later:

  • Likes (Note that the Like button does not yet work in AMP: #9555)
  • Calypsoify
  • Masterbar

Changes proposed in this Pull Request:

  • Add support for AMP Dev Mode in Notes module, eliminating the need to deactivate the module to prevent AMP validation errors in Standard/Transitional modes of the AMP plugin. Revert now-unnecessary change in #10945.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Existing

Testing instructions:

  • Install and activate the AMP plugin v1.3.
  • Check out this branch.
  • There should not be validation errors from the Stats or Notes modules.

Proposed changelog entry for your changes:

  • Added support for AMP Dev Mode in Notes module, eliminating the need to deactivate the module to prevent AMP validation errors in Standard/Transitional modes of the AMP plugin.
@westonruter westonruter referenced this pull request Sep 8, 2019
17 of 17 tasks complete
@jetpackbot

This comment has been minimized.

Copy link

commented Sep 8, 2019

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 27f2494

);
if ( Jetpack_AMP_Support::is_amp_request() ) {
$menu['title'] = "<amp-img src='$img_src_2x' width=112 height=24 layout=fixed alt='$alt' title='$title'></amp-img>";

This comment has been minimized.

modules/notes.php Outdated Show resolved Hide resolved
@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

FYI: I've attached a build of the AMP plugin with a link the PR description at ampproject/amp-wp#3187

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

Here's a writeup of Dev Mode in the AMP plugin and how to integrate with it: https://weston.ruter.net/2019/09/24/integrating-with-amp-dev-mode-in-wordpress/

@jeherve

This comment has been minimized.

Copy link
Member

commented Sep 25, 2019

If this is ready for review, could you mark the PR as such? It's currently set to draft.

Thanks!

@westonruter westonruter marked this pull request as ready for review Sep 25, 2019
@westonruter westonruter requested a review from Automattic/jetpack-crew as a code owner Sep 25, 2019
@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2019

(It was draft because there are some outstanding questions regarding Likes, Calypsoify, and Masterbar.)

@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2019

@jeherve Perhaps we should just deal with those modules later. Having support for Notes (and Stats) would be a good win.

@kraftbj kraftbj dismissed jeherve’s stale review Oct 1, 2019

issue resolved.

Copy link
Member

left a comment

This looks good to me. 👍

westonruter added 3 commits Sep 8, 2019
* Use regular img for admin-bar stats on AMP pages thanks to dev mode
* Add data-ampdevmode attribute to inline style
@westonruter westonruter changed the title Add support for AMP dev mode Add support for AMP Dev Mode in Notes module Oct 2, 2019
@westonruter westonruter force-pushed the westonruter:add/amp-dev-mode-support branch from d5762d1 to 27f2494 Oct 2, 2019
@westonruter westonruter removed their assignment Oct 2, 2019
@westonruter

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2019

@jeherve I've updated the PR description based on the changes being proposed now. This should be good to merge.

@jeherve
jeherve approved these changes Oct 2, 2019
@kraftbj kraftbj merged commit 6dfc387 into Automattic:master Oct 5, 2019
4 checks passed
4 checks passed
triage
Details
codeclimate Approved by Brandon Kraft.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
wpcom No files affecting WordPress.com found.
Details
jeherve added a commit that referenced this pull request Oct 8, 2019
Follow up from #13450

In some cases, admin-bar styles may be dequeued (like when the Masterbar module is active).
kraftbj added a commit that referenced this pull request Oct 9, 2019
* Notifications: do not rely on admin-bar styles

Follow up from #13450

In some cases, admin-bar styles may be dequeued (like when the Masterbar module is active).

* Stop dequeuing style when we need it (when notes is active)

This also adds the dependency back:
#13690 (comment)
jeherve added a commit that referenced this pull request Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.