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

Rebrand Classic as Reader mode and Paired as Transitional mode; add Exit Reader Mode link #2034

Merged
merged 19 commits into from Apr 6, 2019

Conversation

Projects
None yet
6 participants
@felixarntz
Copy link
Collaborator

commented Mar 28, 2019

Fixes #1943. Fixes #2022.

  • Replaces Classic mode with Reader mode in wording.
  • Adjusts relevant user-facing language to better match Reader mode intentions.
  • Renames Paired mode to Transitional mode.
  • Updates description of the modes.
  • Adds a link to the top-right corner of the Reader mode template which points to the same resource in non-AMP version.
  • Fixes erroneous inclusion of AMP panel in Customizer when theme support is present (these settings only apply to Reader mode).

I decided on going with an additional link to the canonical version because using the current comments link for that would be misleading. It points to the comments section of the post, where what we want is just a link to exit Reader mode and view the canonical version. Furthermore, if we just replaced that "Leave a Comment" link with an "Exit Reader Mode" link, it would be an unexpected location for such a link, as you typically add comments below a post.

felixarntz added some commits Mar 28, 2019

@felixarntz felixarntz requested review from westonruter and amedina Mar 28, 2019

@googlebot googlebot added the cla: yes label Mar 28, 2019

</label>
</dt>
<dd>
<?php esc_html_e( 'Display AMP responses in classic (legacy) post templates in a basic design that does not match your theme\'s templates.', 'amp' ); ?>
<?php esc_html_e( 'Display AMP responses in a simplified reader mode design that does not match your theme\'s templates.', 'amp' ); ?>

This comment has been minimized.

Copy link
@felixarntz

felixarntz Mar 28, 2019

Author Collaborator

We might need to discuss how to best phrase this here. Do we want to highlight that it's not respecting the theme's design? Should we maybe phrase this as "separate from your theme" to sound less negative?

This comment has been minimized.

Copy link
@swissspidy

swissspidy Mar 28, 2019

Collaborator

Display AMP responses in a simplified reader mode design separate from your theme could work

This comment has been minimized.

Copy link
@amedina

amedina Mar 30, 2019

Member

Another alternative: Generates AMP content using simplified templates, which are light but may not match the look-and-feel of your site

'post_modified_timestamp' => $post_modified_timestamp,
'post_author' => $post_author,
'post_canonical_link_url' => get_permalink( $this->ID ),
'post_canonical_link_text' => __( 'Exit Reader Mode', 'amp' ),

This comment has been minimized.

Copy link
@felixarntz

felixarntz Mar 28, 2019

Author Collaborator

Alternatively, we could say "View Canonical". What do you think?

This comment has been minimized.

Copy link
@swissspidy

swissspidy Mar 28, 2019

Collaborator

I don't think the average user knows what "Canonical" means in this context. Not to speak of translators. "Original" or "Full Version" would work better though

Exit Reader Mode sounds reasonable though.

For comparison, Safari uses "Hide Reader View".

This comment has been minimized.

Copy link
@amedina

amedina Mar 30, 2019

Member

I see the point of "View Canonical", but it is true that is will be confusing for most users. OTOH, having "Exit Reader Mode" implies that the user is aware that she is in Reader Mode. Probably we can express the duality by a exit route with a message along these lines: "You are viewing the Reader Mode version of <this post>", where <this post> links to the canonical version.

readme.txt Outdated
1. A WP-CLI command is provided to check the URLs on a site for AMP validity. Results are available in the admin for inspection.
2. Many themes can be served as AMP without any changes; the default experience is as if JavaScript is turned off in the browser since scripts are removed.
3. Reader mode templates are still available, but they are are limited. Not only do they differ from the active theme, any validation errors are silently sanitized.
4. Switch from Reader mode to Paired or Native mode in AMP settings screen. You may need to disable the admin bar in AMP if your theme has a larger amount of CSS.

This comment has been minimized.

Copy link
@felixarntz

felixarntz Mar 28, 2019

Author Collaborator

Screenshots 3 and 4 need to be refreshed. @westonruter @ThierryA Do you have the originals there?

This comment has been minimized.

Copy link
@ThierryA

ThierryA Apr 4, 2019

Collaborator

@felixarntz what to do you mean by originals?

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 4, 2019

Member

Original screenshots? Yes. For screenshot 3 I just went to https://2017-theme.amp-wp.org/roads-of-the-city/ and enabled classic mode.

And I took the screenshot of the AMP settings screen from that environment too.

Show resolved Hide resolved readme.md Outdated

@felixarntz felixarntz changed the title Rebrand Classic mode as Reader mode Rebrand Classic as Reader mode and Paired as Transitional mode Mar 28, 2019

@felixarntz

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 28, 2019

I will continue work on this on Monday, and also cover the rebranding of Paired mode, per our earlier conversation.

@amedina
Copy link
Member

left a comment

We should add a configuration option on the Reader mode selection panel, where the user opts in to having the "Exit Reader" mode link added to their templates. This would be important in cases where the addition of the link would interfere with customizations made to classic templates.

felixarntz added some commits Apr 1, 2019

@felixarntz

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 1, 2019

The latest commits take care of rebranding Paired Mode and making the Reader mode exit link opt-in via a Customizer control.

@amedina

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

Another piece of this PR: rebranding of Native mode to Strict AMP Mode.

@amedina

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

I can't see the Exit Reader Mode link anymore, or the configuration option.

@felixarntz

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 2, 2019

@amedina

Another piece of this PR: rebranding of Native mode to Strict AMP Mode.

I think we should have a separate discussion around this and keep this PR focused on Reader and Paired modes. To me, Strict Mode is different from Native Mode:

  • While Native Mode ensures an AMP-compatible experience with auto-sanitization but potentially breaks JS-based content, Strict Mode pretty much provides a turnkey solution, as AMP-incompatible extensions can't be installed.
  • A developer with AMP experience might be unnecessarily restricted by Strict Mode, as they might know that a specific plugin is AMP-compatible even though it's not part of our official AMP whitelist. However, such users would still wanna benefit from Native Mode. I personally would for example fall under that group - I'd wanna use Native Mode, but not Strict Mode.
  • Native Mode can always be enabled, however Strict Mode as we've been envisioning it only really works well on a new site (see our previous WP3 discussions).

I think Strict Mode should either be a fourth mode, or a checkbox that is available when Native Mode is active.

I can't see the Exit Reader Mode link anymore, or the configuration option.

The exit link is now opt-in as you suggested. You can control it in the Customizer together with the other header bar customizations.

$native_description = __( 'Reuses active theme\'s templates to display AMP responses but does not use separate URLs for AMP. This means your site is <b>AMP-first</b> and your canonical URLs are AMP.', 'amp' );
$transitional_description = __( 'Reuses active theme\'s templates to display AMP responses, but uses separate URLs for AMP. Each canonical URL may have a corresponding AMP URL, if the content is fully AMP valid.', 'amp' );
$reader_description = __( 'Generates AMP content using simplified templates, which are light but may not match the look-and-feel of your site.', 'amp' );

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 3, 2019

Member

I think this description should perhaps note something about classic, like “These include the classic templates.”

This also should note that only posts/pages can be served as AMP in this mode. It is not suitable for other kinds of queries.

$theme_support = AMP_Options_Manager::get_option( 'theme_support' );
$native_description = __( 'Reuses active theme\'s templates to display AMP responses but does not use separate URLs for AMP. This means your site is <b>AMP-first</b> and your canonical URLs are AMP.', 'amp' );
$transitional_description = __( 'Reuses active theme\'s templates to display AMP responses, but uses separate URLs for AMP. Each canonical URL may have a corresponding AMP URL, if the content is fully AMP valid.', 'amp' );

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 3, 2019

Member

I think the transitional mode and (especially) reader mode should both include a note that says mobile visitors to origin will not be automatically redirected to the AMP version. The AMP version is presented to mobile users encountering your content via Google Search, Twitter, etc.

readme.txt Outdated
1. Activate the plugin through the 'Plugins' menu in WordPress.
1. If you currently use older versions of the plugin in `Classic mode`, it is strongly encouraged to migrate to `Paired` or `Native mode`.
2. Activate the plugin through the 'Plugins' menu in WordPress.
3. If you currently use older versions of the plugin in `Reader` mode, it is strongly encouraged to migrate to `Transitional` or `Native` mode.

This comment has been minimized.

Copy link
@westonruter

westonruter Apr 3, 2019

Member

The note about the behavior of reader/transitional modes should also be added here, that they are for mobile visitors from Google Search, Twitter, etc. No redirection is done. This is not a mobile theme.

/cc @amedina

This comment has been minimized.

Copy link
@amedina

amedina Apr 5, 2019

Member

Agree. Same line from the Admin Settings page would work.

@westonruter
Copy link
Member

left a comment

@amedina Here is is what the setting looks like:

image

With the exit link displayed:

image

@felixarntz There seems to be a styling issue with the link with the site icon.

@westonruter westonruter added this to the v1.1 milestone Apr 3, 2019

westonruter added some commits Apr 5, 2019

Merge branch 'develop' of github.com:ampproject/amp-wp into add/reade…
…r-mode

* 'develop' of github.com:ampproject/amp-wp: (50 commits)
  Fix printing of PHP upgrade notice
  Fix PHP notice for undefined theme support arg for service_worker
  Remove admin pointer tests that fail in 1.1
  Discontinue showing theme support admin pointer in 1.1
  Add tests for AMP integration with PWA plugin
  Ensure is_amp_endpoint() returns false for service worker requests
  Account for element attributes when determining if a parent is empty and can be removed
  Fix tests after always including Gutenberg
  Add check for argument type in add_google_fonts_caching; advise to update to PWA 0.2
  Align default-enabled service worker features with ABE SW
  Remove todo resolved by xwp/pwa-wp#147
  Make AMP service worker opt-out instead of opt-in
  Opt-in to amp-img-auto-sizes experiment when there are converted images
  Remove obsolete add_auto_width_to_figure from #1086
  Add test for sizes attribute being removed from converted img
  Improve aligncenter, alignwide, and alignfull in classic templates
  Rename method to adjust_twentynineteen_images
  Eliminate sizes from amp-img converted from img
  Ensure featured image gets responsive layout in Twenty Nineteen, as it is essentially alignwide
  Give responsive layout to alignwide/alignfull instead of intrinsic layout
  ...
@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

Updates to description after iterating with @amedina:

image

Outstanding is the styling of the exit link.

westonruter added some commits Apr 5, 2019

@westonruter

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

When site icon is absent:

When site icon is absent

When site icon is present:

When site icon is present

@westonruter westonruter requested a review from amedina Apr 5, 2019

westonruter added some commits Apr 5, 2019

Merge branch 'develop' of github.com:ampproject/amp-wp into add/reade…
…r-mode

* 'develop' of github.com:ampproject/amp-wp:
  Add missing amp-install-serviceworker script in classic mode

@westonruter westonruter changed the title Rebrand Classic as Reader mode and Paired as Transitional mode Rebrand Classic as Reader mode and Paired as Transitional mode; add Exit Reader Mode link Apr 5, 2019

@amedina

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I like the design of the "Exit Reader Mode" style.

westonruter and others added some commits Apr 5, 2019

@westonruter westonruter merged commit e33ba10 into develop Apr 6, 2019

3 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the add/reader-mode branch Apr 6, 2019

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.