Issue 848: Allow theme support of canonical AMP #852
Conversation
If themes register this with add_theme_support( 'amp' ), the plugin will be in 'canonical mode.' It won't display the 'rel' link, And the theme will still use its own template. But if any other argumnents are passed to add_theme_support( 'amp', It will retain 'paired mode.' Add unit tests for this new feature.
Great point. No, this should not be kept when in canonical mode. |
| } | ||
| if ( is_array( $support ) ) { | ||
| $args = array_shift( $support ); | ||
| if ( empty( $args['template_path'] ) ) { |
There was a problem hiding this comment.
The use of template_path will probably need to be iterated upon. For example, theme_root per \WP_Theme::get_theme_root() or template_dir per \WP_Theme::get_template_directory(). This will need some more discovery in #849 as to what makes the most sense. To me it seems the theme_root and template_directory are the same. /cc @kaitnyl
amp.php
Outdated
| @@ -128,7 +137,7 @@ function amp_maybe_add_actions() { | |||
| global $wp_query; | |||
| $post = $wp_query->post; | |||
|
|
|||
| $supports = post_supports_amp( $post ); | |||
| $supports = post_supports_amp( $post ) && ! amp_is_canonical(); | |||
There was a problem hiding this comment.
Might as well move the ! amp_is_canonical() check up to the top of the function:
if ( amp_is_canonical() || ! is_singular() || is_feed() ) {Add test for amp_is_canonical() when arg other than template_path is added
|
Thanks For Reviewing Hi @westonruter, |
@kienstra This could be a bit tricky because without the post types settings section, the AMP > General admin page would have nothing on it. Maybe for now the best thing would be to just prevent rendering that settings section and instead show a message that canonical AMP is enabled, and so all post types will be shown regardless. Or rather, modify the section to display every post type with the checkbox |
If the theme registers support for canonical AMP, Check all of the post types in the 'AMP Settings' page. And disable their <input>, as the theme will render the post types. Also, change the description at the bottom. And don't output the 'submit' button in canonical mode.
|
Applied Suggestion For 'AMP Settings' Hi @westonruter, Here's the 'AMP Settings' page when the theme registers support for AMP 'canonical mode': |
Request For Review
Hi @ThierryA or @westonruter,
Could you please review this pull request for Issue #848?
If the theme runs
add_theme_support( 'amp' )without any other arguments, the plugin will be in 'canonical mode.' The theme will still use its own template(s).But if any other argumnents are passed to
add_theme_support( 'amp' ), it will retain 'paired mode.' That case is in #849.When in 'canonical mode':
post.phppage's 'Publish' meta box doesn't have the AMP preview link, nor the "AMP: Enabled" section:Also, should we keep the 'AMP Settings' page?
When you add or remove support for a custom post type,
amp_is_canonical()short-circuits this if it istrue, and prevents the plugin from using its own templates and running frontend actions.Fixes #848.