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

Prototype Reader themes #4644

Closed
wants to merge 69 commits into from
Closed

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented May 2, 2020

Summary

This PR introduces the ability to select from among AMP-compatible themes to be used when in Reader mode, in addition to a “Classic” theme which behaves as the current Reader mode. If not using Classic, then selecting a theme allows for all templates on a site to be used as AMP, including singular, homepage, archives, etc.

  • Being able to select a Reader theme is dependent on a site not customizing the AMP endpoint (query var). So if a site changes the query var from amp to something else like lite, then a warning is displayed instead of the theme list.
  • The listed themes exclude the active theme.
  • When the active theme is switched to be the same as the theme that is selected for the Reader theme, then the template mode is automatically switched from Reader to Transitional.
  • Enable developer tools in Reader mode, along with a “Dev Tools” toggle to the admin settings screen. Note that this is at an intermediary stage, as eventually the Dev Tools toggle should be user-specific.

Customizer

  • At the moment, the Customizer for the selected Reader theme can be accessed by clicking “Customize” in the admin bar when viewing an AMP page on the frontend. (There will need to be a Customize button added to the AMP admin settings screen.)
  • The AMP panel in the Customizer which is normally shown in Reader mode is omitted entirely when a Reader theme has been selected, since the Customizer is loaded with that Reader theme loaded in its entirety. The AMP toggle appearing next to the device switcher is not shown when a Reader theme is selected; this is only applicable to classic Reader mode. When a Reader theme is selected, the Customizer loaded up-front with either the active theme or the reader theme applied.
  • Theme-specific customizations (e.g. color scheme, custom header, background) are only applied to the selected theme, leaving the active theme's settings intact.
  • Nav menus can be fully managed in AMP. Nav menu locations are specific to the Reader theme or active theme, but the menus assigned to those locations are shared between the AMP and non-AMP versions of the sites.
  • Widgets are disabled in Reader themes; this includes hiding the Widgets panel in the Customizer. Since WordPress lacks the concept of “sidebar locations” as it has with “nav menu locations”, being able to manage widgets in Reader mode would be very complicated. Since widgets are not deemed necessary for Reader mode (especially given the mobile viewport), they are disabled.
  • The Themes panel is removed when opening AMP Customizer for a Reader theme. Selecting a different Reader theme must be done currently from the AMP settings screen.
  • When the Customizer is opened for a Reader theme, then the previewed device will default to mobile.

Addresses issue #4560

Screenshots

When a Reader theme is selected, you can choose from among the installed themes which are AMP-compatible. Currently this includes the core themes:

image

When the Classic theme is selected, templates are not available to be selected and a notice is explained why.

image

Opening the Customizer from an AMP page will hide the themes panel, widgets panel (since widgets are disabled), default to the mobile device, and include a note indicating the AMP version of the site is being customized:

image

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@googlebot googlebot added the cla: yes Signed the Google CLA label May 2, 2020
@westonruter westonruter requested a review from amedina May 2, 2020 06:46
@westonruter
Copy link
Member Author

5. And the customizer settings I had made in Reader mode (i.e. for the TwentyTwenty theme) were gone
If I manually switch the main site back to the original theme, I m able to select TwentyTwenty again for Reader mode (as expected), and the customizer settings made before the switch are there.

Yes, this is expected. The theme customizations are stored with the theme itself. WordPress doesn't share theme customizations across themes, not even between parent and child themes.

@westonruter
Copy link
Member Author

Add some indication of the Customizer controls for settings that are shared between the AMP and non-AMP sites (i.e. options) and then for settings which are theme-specific, provide a way to import those specific settings from the active theme.

westonruter and others added 15 commits May 12, 2020 09:18
…n-suppression

* 'develop' of github.com:ampproject/amp-wp:
  Strip multiple BOM characters (#4683)
  Strip leading BOM and whitespace and trailing HTML comment before parsing validation response JSON (#4679)
  Update dependency xwp/wp-dev-lib to v1.6.4
  Update dependency eslint to v7
…/reader-mode-themes

* 'develop' of github.com:ampproject/amp-wp: (59 commits)
  Strip multiple BOM characters (#4683)
  Strip leading BOM and whitespace and trailing HTML comment before parsing validation response JSON (#4679)
  Update dependency xwp/wp-dev-lib to v1.6.4
  Update dependency eslint to v7
  Remove unused RuntimeException
  Use explicit string type instead of generic array in return
  Avoid method map and replace with a switch
  Defer removal of attributes until after layout application
  Fix post preview in Reader mode (#4665)
  Ensure that source map comment is preserved at the end of amp-custom.css
  Pass DOMAttr instances rather than attribute name and value as separate args
  Enable SSR by default, remove use of WP_DEBUG as a signal. (#4669)
  Use padding-top and override regular inline style for heights
  Update php-parallel-lint dependency to new name
  Use :first-child to reference sizer in CSS
  Update dependency @wordpress/babel-preset-default to v4.12.1 (#4494)
  Fix error type for bad sizes test
  Provide more precise error for thrown exception
  Test precise error messages
  Fix attribute name in error messages
  ...
* add/plugin-suppression:
  Override widget callback to return null instead of empty string (since widgets print)
  Omit error duplicates from suppressed plugin list
  Omit suppressed plugins section if no erroring/suppressed plugins; omit others list
  Re-check a validated URL after changing suppressed plugins
  Pass suppressed plugins as params
  Ensure plugin suppression only happens on AMP pages
  Fix test failing with undefined index
  Suppress plugin widgets
  Remove redundancy in checking callback sources
  Suppress plugin blocks
  Add suppression of plugin shortcodes
  Add initial plugin suppression logic for hooks
  Store plugin slugs instead of files for suppressed list
  Hide non-suppressant plugins and show validation errors
  Ensure get_plugins() function is loaded
  Include plugin versions in the validated environment
  Keep track of the plugin version which was suppressed
  Add suppressed plugins option
…/reader-mode-themes

* 'develop' of github.com:ampproject/amp-wp: (53 commits)
  Add tests for tweetid binding, Google Fonts v2, and enterkeyhint
  Update tests to reflect removal of style attribute constraint
  Adapt docblocks
  Display PHPStan version in Travis
  Update dependency eslint-plugin-jest to v23.13.1
  Adapt in libraries as well
  Always download latest version of PHPStan
  Update dependency autoprefixer to v9.8.0
  Update dependency eslint-plugin-jest to v23.13.0
  Fix phpdoc for sanitizer_classes and embed_handler_classes
  Remove array_column() polyfill since plugin requires PHP 5.6+
  Update dependency @wordpress/scripts to v9.1.0
  Remove redundant subtraction
  Raise level to 3
  Adapt type-hint for AMP_Image_Dimension_Extractor::normalize_url()
  Use empty string for pruning textContent
  Adapt typehint for AMP_Content->sanitizer_classes
  Adapt typehint for AMP_Tag_And_Attribute_Sanitizer->allowed_tags
  Adapt typehints for methods returning a rule spec check result
  Adapt typehint for AMP_Style_Sanitizer->current_sources
  ...
…n-suppression

* 'develop' of github.com:ampproject/amp-wp: (71 commits)
  Eliminate needless variable
  Typecast `display_errors` config before comparing value
  Add tests for tweetid binding, Google Fonts v2, and enterkeyhint
  Update tests to reflect removal of style attribute constraint
  Suppress display of fatal errors during validate request
  Adapt docblocks
  Display PHPStan version in Travis
  Ensure boolean return value for isAMPEnabled
  Fix logic for determining which post types should get REST API fields
  Give explicit null return value for update_amp_enabled_rest_field
  Update method names, comments, and add versions
  Update dependency eslint-plugin-jest to v23.13.1
  Adapt in libraries as well
  Always download latest version of PHPStan
  Update dependency autoprefixer to v9.8.0
  Update dependency eslint-plugin-jest to v23.13.0
  Get eligible post types from `AMP_Post_Type_Support::get_post_types_for_rest_api()`
  Register REST field for `page` post type
  Fix phpdoc for sanitizer_classes and embed_handler_classes
  Remove array_column() polyfill since plugin requires PHP 5.6+
  ...
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
…n-suppression

* 'develop' of github.com:ampproject/amp-wp: (67 commits)
  Update reasoning behind omitting type=“text/html” attribute
  Update dependency @wordpress/block-editor to v3.11.0 (#4725)
  Update dependency eslint to v7.1.0
  Implement suggested changes and test case
  Omit “type” attribute during “<amp-iframe>” normalization
  Check that var isset before accessing from
  AMP setup screen - set version and dependencies from asset file
  Replace use of filter_input
  Update webpack.config.js
  Get version from bloginfo instead of global
  Use align-wide theme support check to use responsive layout
  Update dependency @wordpress/blocks to v6.16.0 (#4462)
  Update dependency @wordpress/components to v9.6.0 (#4463)
  Update dependency @wordpress/data to v4.18.0 (#4496)
  Update dependency @babel/core to v7.9.6 (#4636)
  Update dependency @wordpress/babel-preset-default to v4.13.0 (#4724)
  Remove extraneous arg
  Remove unused var
  Remove script overrides on backend; bundle everything
  Update includes/options/views/class-amp-setup-wizard-submenu-page.php
  ...
… into prototype/reader-mode-themes

* 'add/plugin-suppression' of github.com:ampproject/amp-wp: (92 commits)
  Remove superflous line-through styling
  Update reasoning behind omitting type=“text/html” attribute
  Update dependency @wordpress/block-editor to v3.11.0 (#4725)
  Update dependency eslint to v7.1.0
  Implement suggested changes and test case
  Omit “type” attribute during “<amp-iframe>” normalization
  Check that var isset before accessing from
  AMP setup screen - set version and dependencies from asset file
  Replace use of filter_input
  Update webpack.config.js
  Get version from bloginfo instead of global
  Use align-wide theme support check to use responsive layout
  Update dependency @wordpress/blocks to v6.16.0 (#4462)
  Update dependency @wordpress/components to v9.6.0 (#4463)
  Update dependency @wordpress/data to v4.18.0 (#4496)
  Update dependency @babel/core to v7.9.6 (#4636)
  Update dependency @wordpress/babel-preset-default to v4.13.0 (#4724)
  Fix PHPStan analysis by unsetting WP_Block_Type script and style instead of setting to null
  Include options in validated environment and staleness check
  Fix syntax error after ccbf771
  ...
@westonruter
Copy link
Member Author

Closing in favor of #4984.

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
Development

Successfully merging this pull request may close these issues.

3 participants