Implement ReaderThemeLoader service to load selected Reader theme for AMP pages#4984
Merged
westonruter merged 38 commits intodevelopfrom Jul 8, 2020
Merged
Implement ReaderThemeLoader service to load selected Reader theme for AMP pages#4984westonruter merged 38 commits intodevelopfrom
westonruter merged 38 commits intodevelopfrom
Conversation
… AMP pages
* Reuse Customizer logic to switch the active theme when a Reader theme is selected.
* Add Reader theme to Site Health info.
* Open AMP Customizer when clicking the Customize admin bar menu item on an AMP page in the new Reader mode.
* Add `amp_is_legacy()` helper function which is used to determine if legacy Reader mode is being used. This is essentially the same as `!current_theme_support('amp')`.
* Split out Customizer JS and CSS for legacy Reader mode.
* Limit extending Customizer if Legacy Reader mode is active or if requesting Customizer with query var.
* Eliminate use of `amp` theme support to indicate whether Standard or Transitional mode are active; rely on option alone, except for narrow case of the default value. Deprecate various methods used for this purpose, including `read_theme_support`.
* Eliminate the long-deprecated active_callback theme support arg.
* Ensure AMP Customizer admin menu link navigates to a supported URL by default.
* Ensure that `is_amp_available()` always returns true if in Customizer preview and new Reader mode is enabled.
* Add reader theme to meta generator tag.
westonruter
commented
Jul 4, 2020
|
|
||
| /* eslint no-magic-numbers: [ "error", { "ignore": [ 0, 1, 250] } ] */ | ||
|
|
||
| window.ampCustomizeControls = ( function( api, $ ) { |
Member
Author
There was a problem hiding this comment.
This file is simply renamed from amp-customize-controls.js. No changes have been made otherwise.
Contributor
|
Plugin builds for 8bd2038 are ready 🛎️!
|
westonruter
commented
Jul 4, 2020
| @@ -132,7 +132,7 @@ public function add_menu_items() { | |||
| * @since 1.0 | |||
| */ | |||
| public function render_theme_support() { | |||
Member
Author
There was a problem hiding this comment.
@johnwatkins0 FYI: These changes here are incomplete as I expect you'll be revamping everything as part of #4704.
This comment has been minimized.
This comment has been minimized.
…r-theme-loader * 'develop' of github.com:ampproject/amp-wp: Remove last references to allow_dirty_styles and allow_dirty_scripts Reduce indentation Remove unused sanitizer args Ensure all dependencies of the `customizer-preview` style handle are not sanitized Prevent sanitization of Customizer preview styles
2c04ccd to
7943dc6
Compare
This comment has been minimized.
This comment has been minimized.
westonruter
added a commit
that referenced
this pull request
Jul 4, 2020
Note that this will need to be restored when in non-legacy Reader mode as part of #4984.
3 tasks
…r-theme-loader * 'develop' of github.com:ampproject/amp-wp: (49 commits) Escape admin bar item title text Add Settings link to the AMP admin bar item when Dev Tools enabled Remove needless conditional Handle all removed and accepted case in same way as otherwise Make validate link text more concise and consistent Improve displaying error counts in validate link Show warning in admin bar when kept invalid markup in standard mode Use empty() instead of isset() Update 'Validate' admin bar item link to 'Validate URL' Fix lint issue Set mobile_redirect to true from `originalOptions` when value is unchanged in first run Remove previously completed todo Optimize AMP_Tag_And_Attribute_Sanitizer::get_missing_mandatory_attributes() Run astra theme e2e tests separately so theme can be activated/removed only once Recommend transitional/reader mode to technical users with amp-compatible active theme Fix Jest tests Onboarding wizard close link: return to previous location Persist default mobile_redirect setting when it is not interacted with Prevent wpfooter from preventing clicks Fix lint issues ...
…itches to Reader theme
…r-theme-loader * 'develop' of github.com:ampproject/amp-wp: Bump stable tag to 1.5.5
5 tasks
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
Contributor
|
Re this point in the changelog:
Should
|
Member
Author
It's a good question, and I thought about that, but I decided to leave it as a “power user” feature. Also, we may want to allow paired browsing in Reader mode. To do that, we'll need to introduce a toggle for enabling/disabling scroll locking. In Transitional mode, the toggle would be on by default. In Reader mode, it would be off by default. |
Member
Author
|
Todos complete! |
Contributor
|
Issue with legacy reader mode:
|
Contributor
|
Nevermind! Just forgot to rebuild the JS scripts 😅 . |
Contributor
|
My first point from #4984 (comment) still seems valid, however. |
Contributor
|
The AMP toggle is still on when navigating to a non-singular template; I can also toggle it on said template:
|
Member
Author
This would be the same point as you raised in #4984 (comment). Let me make your suggestion to limit that logic to when legacy Reader mode is enabled. |
Member
Author
I believe this would be an existing issue not introduced by this PR, is that right? |
Contributor
|
I don't think so. I just did a quick test on the |
a008c15 to
d9c8de3
Compare
Member
Author
I see what happened. Fixed in a008c15! |
pierlon
suggested changes
Jul 8, 2020
Co-authored-by: Pierre Gordon <16200219+pierlon@users.noreply.github.com>
This was referenced Jul 8, 2020
schlessera
reviewed
Jul 9, 2020
| public function register() { | ||
| // The following needs to run at plugins_loaded because that is when _wp_customize_include runs. Otherwise, the | ||
| // most logical action would be setup_theme. | ||
| add_action( 'plugins_loaded', [ $this, 'override_theme' ], 9 ); |
Collaborator
There was a problem hiding this comment.
This should use the Delayed interface instead, and then override_theme would be the new register.
Member
Author
There was a problem hiding this comment.
Not quite, as it has to happen specifically at priority 9. The Delayed interface only provides for the action name generally, so that would mean priority 10.
| $theme_support = AMP_Options_Manager::get_option( Option::THEME_SUPPORT ); | ||
|
|
||
| /** @var AmpSlugCustomizationWatcher $amp_slug_customization_watcher */ | ||
| $amp_slug_customization_watcher = Services::get( 'amp_slug_customization_watcher' ); |
Collaborator
There was a problem hiding this comment.
This should be injected instead, provided that AMP_Options_Menu is instantiated via the injector.
Member
Author
There was a problem hiding this comment.
Agreed. Since AMP_Options_Menu is not yet instantiated that way, I used Services in the time being.
| } | ||
|
|
||
| /** @var AmpSlugCustomizationWatcher $amp_slug_customization_watcher */ | ||
| $amp_slug_customization_watcher = Services::get( 'amp_slug_customization_watcher' ); |
Collaborator
There was a problem hiding this comment.
This should be injected instead, provided that AMP_Setup_Wizard_Submenu_Page is instantiated via the injector.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4478.
Fixes #4476.
Fixes #4708.
Closes #2044.
See #4560.
See #4475.
This PR introduces the ability to select from among AMP-compatible themes to be used when in Reader mode, in addition to a “Legacy” theme which behaves as the current Reader mode. If not using legacy, then selecting a theme allows for all templates on a site to be used as AMP, including singular, homepage, archives, etc.
👉 The UI for selecting the Reader theme is limited to the wizard. The ability to select the Reader theme from the admin screen will be implemented via #4560 as part of #4704. Of particular note, the ability to select individual templates will need to be allowed in Reader mode when a non-legacy template is selected.
Customizer
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:
Changelog
readertotransitional. This is necessary because Reader mode with the a Reader theme selected which is the same as the active theme is no different than Transitional mode. If the active theme is switched then to a different theme, then Reader mode is resumed.amp_is_legacy()helper function which is used to determine if legacy Reader mode is being used. This is essentially the same as!current_theme_support('amp').amptheme support to indicate whether Standard or Transitional mode are active; rely on option alone, except for narrow case of the default value. Deprecate various methods used for this purpose, includingread_theme_support. Nevertheless, when serving a page with legacy Reader mode, make sure theamptheme support is removed; conversely, when serving a non-legacy AMP page ensure theamptheme support is present. Some plugins checkcurrent_theme_supports('amp')in order to determine whether they should use standard WordPress theme hooks or else the hooks for the legacy AMP post templates.is_amp_available()always returns true if in Customizer preview and new Reader mode is enabled.themetag (e.g.mode=reader; theme=legacyormode=reader; theme=twentynineteen).AMP_CUSTOMIZER_QUERY_VARconstant.Cherry-picks logic from prototype PR #4644.
Todo
current_theme_supports('amp')when serving Standard/Transitional mode for compatibility with plugins that look at this: https://wpdirectory.net/search/01ECKV6G437XCTS2ZMX0QBT0JKampto something else likelite, then a warning needs to be displayed. Same goes for a theme registering a custom post type, etc.ReaderThemeLoader.Checklist