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

Refactor AMP modes #2550

Merged
merged 21 commits into from
Jun 11, 2019
Merged

Refactor AMP modes #2550

merged 21 commits into from
Jun 11, 2019

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jun 8, 2019

This PR intends to refactor the AMP modes in the plugin in order to resolve three issues:

Always allow switching themes with built-in Transitional support to Native

When a theme had add_theme_support( 'amp', array( 'paired' => true ) ) they would see:

image

Now, they see this instead:

image

This allows a theme which declares support for both AMP and non-AMP, to just opt to only serve AMP pages instead of being paired mode. This allows a theme like Neve to be used in either mode, even though it is defined as being paired => true.

Additionally, if they had add_theme_support( 'amp' ) then before they would see:

image

Now they see a slightly tweaked message:

image

Notice the link to the ecosystem page is moved to the bottom.

Generator Tag

Given a site in transitional mode with both website and stories experiences enabled, the generator tag is now:

<meta name="generator" content="AMP Plugin v1.2-beta2; mode=transitional; experiences=website,stories">

Note the new experiences param. If switching to standard (formerly native) mode, this is also reflected:

<meta name="generator" content="AMP Plugin v1.2-beta2; mode=standard; experiences=website,stories">

If stories is disabled, then this is shown as:

<meta name="generator" content="AMP Plugin v1.2-beta2; mode=standard; experiences=website">

If website is disabled but stories is enabled, then:

<meta name="generator" content="AMP Plugin v1.2-beta2; mode=none; experiences=stories">

@westonruter westonruter added this to the v1.2 milestone Jun 8, 2019
@westonruter westonruter requested a review from amedina June 8, 2019 20:19
@googlebot googlebot added the cla: yes Signed the Google CLA label Jun 8, 2019
@amedina
Copy link
Member

amedina commented Jun 8, 2019

Excellent. These changes increase both user friendliness and flexibility. Kudos on removing the option for Reader mode when Transitional mode is added via theme support.

@amedina
Copy link
Member

amedina commented Jun 9, 2019

This allows a theme which declares support for both AMP and non-AMP, to just opt to only serve AMP pages instead of being paired mode.

This means users can go from Transitional to Native also in all cases. It would be good to allow users in Transitional mode to chose Native AMP for some pages. This would enable sites to track progress of the "AMPlification" of their content ~ Your site is 86% Native AMP, 14% Transitional or Your site is 86% Native AMP, 10% Transitional, 4% no-AMP.

@amedina
Copy link
Member

amedina commented Jun 9, 2019

And when Bento AMP is a thing, the scores can be detailed at an even higher granular level.

@westonruter
Copy link
Member Author

westonruter commented Jun 9, 2019

Chosing AMP-first for some URLs but paired for others is somewhat described under the “Mixed-Some-Only-AMP” mode in #934

IMO that adds a lot of complexity that I highly doubt users would be likely to leverage. Possibly some complicated changes to validation logic as well. Unsure how scores would be calculated.

Decisions not options.

I think we should help users get to AMP-first on URLs that support it. For other templates/URLs, non-AMP would be served unless the theme is using Bento AMP components.

@amedina
Copy link
Member

amedina commented Jun 9, 2019

I don't think the complexity will be much higher; and the benefit will be significant especially if weighed by the benefit of being able to measure the progression of a site towards AMP first.

And it also captures the essence of a Transitional mode: the site is converted progressively to AMP first.

The current Transitional approach is to wait until full parity (visual, functional) is achieved for all templates, before the "AMP-first" switch is turned. In this case, 95% of the site can be ready to switch, but it needs to wait for the rest to achieve full parity.

@westonruter westonruter force-pushed the update/theme-support-modes branch from a9f7409 to 6f8a8d8 Compare June 10, 2019 19:37
@westonruter westonruter marked this pull request as ready for review June 10, 2019 20:02
@westonruter westonruter requested a review from kienstra June 10, 2019 20:37
@amedina
Copy link
Member

amedina commented Jun 10, 2019

This means users can go from Transitional to Native also in all cases. It would be good to allow users in Transitional mode to chose Native AMP for some pages. This would enable sites to track progress of the "AMPlification" of their content ~ Your site is 86% Native AMP, 14% Transitional or Your site is 86% Native AMP, 10% Transitional, 4% no-AMP.

Closing the loop on this. We agree that a more fluid model for the Transitional mode would facilitate the transition of sites towards AMP-first experiences by avoiding the current all-or-nothing approach. However, we will be able to achieve this in a much easier way once Bento AMP is available. With Bento AMP a fully fluid transitional mode can be achieved without a lot of the complexity.

amp.php Show resolved Hide resolved
amp.php Outdated Show resolved Hide resolved
Copy link
Member

@amedina amedina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor comments. Ready to ship.

@westonruter
Copy link
Member Author

@amedina FYI, in 9737cd7 I've made it so that stories always have auto-accepting sanitization, and along with that I've shortened the warning to remove wording that is not relevant in non-website experience:

image

@westonruter westonruter requested a review from swissspidy June 11, 2019 05:11
@swissspidy swissspidy merged commit bbecffc into develop Jun 11, 2019
@swissspidy swissspidy deleted the update/theme-support-modes branch June 11, 2019 12:13
westonruter added a commit that referenced this pull request Jun 11, 2019
…ide-lighter-media

* 'develop' of github.com:ampproject/amp-wp:
  Update dependency css-loader to v3 (#2563)
  Consistently import components from block-editor package (#2561)
  Refactor AMP modes (#2550)
  Update dependency webpack-cli to v3.3.4 (#2558)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
5 participants