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

Issue 848: Allow theme support of canonical AMP #852

Merged
merged 6 commits into from Jan 11, 2018

Conversation

Projects
None yet
3 participants
@kienstra
Copy link
Collaborator

kienstra commented Jan 10, 2018

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':

  • There's no Customizer 'AMP' panel
  • There's no AMP 'rel' link
  • The plugin doesn't use its template system
  • The post.php page's 'Publish' meta box doesn't have the AMP preview link, nor the "AMP: Enabled" section:

removed-in-canonical

Also, should we keep the 'AMP Settings' page?
post-type-support

When you add or remove support for a custom post type, amp_is_canonical() short-circuits this if it is true, and prevents the plugin from using its own templates and running frontend actions.

Fixes #848.

Issue 848: Allow theme support of canonical AMP.
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.
@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 10, 2018

Also, should we keep the 'AMP Settings' page?

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'] ) ) {

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 10, 2018

Member

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();

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 10, 2018

Member

Might as well move the ! amp_is_canonical() check up to the top of the function:

if ( amp_is_canonical() || ! is_singular() || is_feed() ) {
Improve position of canonical check in amp_maybe_add_actions()
Add test for amp_is_canonical() when arg other than template_path is added
@westonruter
Copy link
Member

westonruter left a comment

Only thing left is to disable the post type settings section, but otherwise looks great.

@kienstra

This comment has been minimized.

Copy link
Collaborator Author

kienstra commented Jan 10, 2018

Thanks For Reviewing
Will Disable 'AMP Settings' Section

Hi @westonruter,
Thanks a lot for reviewing this. I'll remove the "AMP Settings" post type section.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jan 10, 2018

Thanks a lot for reviewing this. I'll remove the "AMP Settings" post type section.

@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 checked and disabled along with the notice and without the submit button.

Issue 848 : Change 'AMP Settings' page with canonical AMP.
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.
@kienstra

This comment has been minimized.

Copy link
Collaborator Author

kienstra commented Jan 11, 2018

Applied Suggestion For 'AMP Settings'

Hi @westonruter,
Thanks for your suggestion for the 'AMP Settings' page, which the commit above applies.

Here's the 'AMP Settings' page when the theme registers support for AMP 'canonical mode':
theme-registers-support-canonical-amp

@westonruter westonruter merged commit 34a84d9 into develop Jan 11, 2018

2 checks passed

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/848-theme-support-canonical branch Jan 11, 2018

@westonruter westonruter added this to the v0.7 milestone Jan 11, 2018

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.