AMP compatibility #155
AMP compatibility #155
Conversation
functions.php
Outdated
| // Don't include js on AMP endpoints. | ||
| if ( ! twentynineteen_amp_enabled ) { | ||
| wp_enqueue_script( 'twentynineteen-skip-link-focus-fix', get_template_directory_uri() . '/js/skip-link-focus-fix.js', array(), '20151215', true ); | ||
| } |
There was a problem hiding this comment.
This will need to change as of #47.
functions.php
Outdated
| @@ -117,7 +125,10 @@ function twentynineteen_content_width() { | |||
| function twentynineteen_scripts() { | |||
| wp_enqueue_style( 'twentynineteen-style', get_stylesheet_uri() ); | |||
|
|
|||
| wp_enqueue_script( 'twentynineteen-skip-link-focus-fix', get_template_directory_uri() . '/js/skip-link-focus-fix.js', array(), '20151215', true ); | |||
| // Don't include js on AMP endpoints. | |||
| if ( ! twentynineteen_amp_enabled ) { | |||
There was a problem hiding this comment.
Missing parens on the function call.
functions.php
Outdated
| /** | ||
| * Check if AMP plugin is enabled and if AMP endpoint. | ||
| */ | ||
| function twentynineteen_amp_enabled() { |
There was a problem hiding this comment.
In WP Rig this would be called twentynineteen_is_amp(). I think that is better than ...amp_enabled because your site may have AMP enabled even though it is not being served at the moment, as in the case of paired mode or if you opt-out of AMP for a given post.
| * thus it does not warrant having an entire dedicated blocking script being loaded. | ||
| * | ||
| */ | ||
| function _twentynineteen_skip_link_focus_fix() { |
There was a problem hiding this comment.
This change shouldn't be included yet. It's dependent on #47. Also, once/if that PR is merged, this function will then need the is_amp as seen in the WP Rig PR: wprig/wprig#139
There was a problem hiding this comment.
@westonruter should we just revert back to the previous for now until #47 is merged?
| @@ -14,7 +14,7 @@ | |||
| <html <?php language_attributes(); ?>> | |||
| <head> | |||
| <meta charset="<?php bloginfo( 'charset' ); ?>" /> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | |||
| <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1"> | |||
There was a problem hiding this comment.
I've opened a separate PR just for this one change: #199
|
We ended up not adding explicit support for AMP to this theme. To do this properly there needs to be a method to enqueue scripts asynchronously ( See: #94 Since we're closing out this repo to move the theme development fully to the Core Trac, I'm going to close this issue. Thanks! |
This is just a start on #94 , I will keep working on this PR as the theme develops to check the theme is AMP compatible. These changes just resolve some AMP validation issues popping up in the validation checker.
twentynineteen_amp_enabled()function to check whether response is being served as AMP and plugin is enabled.skip-link-focus-fix.json AMP responses since it is not allowed in AMP.minimum-scale=1is required in AMP but probably not desirable in non-AMP cases.Fixes #94.