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

Add initial canonical AMP support #857

Merged
merged 9 commits into from Jan 12, 2018

Conversation

Projects
None yet
3 participants
@westonruter
Copy link
Member

commented Jan 12, 2018

  • Key wp_head and wp_footer actions are removed. See #850.
  • AMP spec requirements are output via wp_head.
  • Theme's style.css and Custom CSS are printed in <style amp-custom>.
  • Canonical link rel is output for singular and non-singular requests alike.
  • Stub added for implementing paired mode for themes that support amp. See #849.
  • Adds PHPCompatibility sniffs for PHPCS and installs via Composer.

For a theme that makes use of these changes, see: xwp/ampnews#17

Todo:

  • Add tests.
@ThierryA
Copy link
Collaborator

left a comment

@westonruter find the CR below. I believe most todos will be addressed in future PR.

Will do another CR once tests are pushed.

*
* @link https://www.ampproject.org/docs/reference/spec#boilerplate
*/
public static function print_amp_boilerplate_code() {

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 12, 2018

Collaborator

This is a duplicate of this method, it will make it difficult to maintain.

add_action( 'wp_head', array( __CLASS__, 'rel_canonical' ), 1 );
// Add additional markup required by AMP <https://www.ampproject.org/docs/reference/spec#required-markup>.
add_action( 'wp_head', array( __CLASS__, 'print_meta_charset' ), 0 );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 12, 2018

Collaborator

Shouldn't this apply to paired mode too?

add_action( 'wp_head', array( __CLASS__, 'print_meta_viewport' ), 2 );
add_action( 'wp_head', array( __CLASS__, 'print_amp_boilerplate_code' ), 3 );
add_action( 'wp_head', array( __CLASS__, 'print_amp_scripts' ), 4 );
add_action( 'wp_head', array( __CLASS__, 'print_amp_custom_style' ), 5 );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 12, 2018

Collaborator

Thinks like add_schemaorg_metadata(), add_analytics_data() or add_generator_metadata() added in paired mode are not added in canonical mode, is that intentional?

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 12, 2018

Author Member

Just not implemented yet.

*/
add_filter( 'show_admin_bar', '__return_false', 100 );
add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ) );

This comment has been minimized.

Copy link
@ThierryA

ThierryA Jan 12, 2018

Collaborator

We could probably add a lower priority to make sure we catch everything, just in case a template is loading using the template_redirect (I have seen that many times) even though it should be using the template_include filter.

This comment has been minimized.

Copy link
@westonruter

westonruter Jan 12, 2018

Author Member

Yeah, good point.

westonruter added some commits Jan 12, 2018

Move AMP boilerplate code to global function for reuse
Start output buffering at template_redirect priority 0

@westonruter westonruter merged commit f2b1831 into develop Jan 12, 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/canonical-head branch Jan 12, 2018

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

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2018

This is just @todo note to add tests for this PR as per discussed via PM. CC @westonruter

@westonruter

This comment has been minimized.

Copy link
Member Author

commented Jan 16, 2018

@ThierryA I've added a dedicated issue for this: #868.

@ThierryA

This comment has been minimized.

Copy link
Collaborator

commented Jan 16, 2018

Great, thanks @westonruter 👍

@kopepasah

This comment has been minimized.

Copy link

commented on includes/actions/class-amp-canonical-mode-actions.php in 6ef3f86 Jan 19, 2018

@westonruter we definitely need to honor get_stylesheet_uri(), as this makes working with a child theme impossible.

This comment has been minimized.

Copy link

replied Jan 19, 2018

Once consideration is users often use @import in the child theme stylesheet to get the parent theme stylesheet, which is not compatible with AMP. We probably will have to parse the stylesheets, remove imports and get the file's contents another way.

This comment has been minimized.

Copy link
Member Author

replied Jan 19, 2018

we definitely need to honor get_stylesheet_uri(), as this makes working with a child theme impossible.

The main issue I see with this is that it is possible (though unlikely I suppose) that get_stylesheet_uri() returns a URL that points outside the WP file system. An external request would not be feasible as it would slow down the page load. But we could just make it a requirement that get_stylesheet_uri() start with get_stylesheet_directory_uri() or get_template_directory_uri().

Once consideration is users often use @import in the child theme stylesheet

Actually, using @import is now discouraged when building child themes. So we can safely require that if a theme says it supports amp that it not use @import. See Codex creating Child Themes:

The final step is to enqueue the parent and child theme stylesheets. Note that the previous method was to import the parent theme stylesheet using @import: this is no longer best practice, as it increases the amount of time it takes style sheets to load. The correct method of enqueuing the parent theme stylesheet is to add a wp_enqueue_scripts action and use wp_enqueue_style() in your child theme's functions.php.

This comment has been minimized.

Copy link

replied Jan 19, 2018

Regarding @import: Awesome!.

I agree that making sure it is on the file system should be a requirement, though I still see instances where using get_template/stylesheet_directory() causes an issue for developers. At the very least, there should be a filter in AMP to change the location (if we are hard coding style.css).

This comment has been minimized.

Copy link
Member Author

replied Jan 20, 2018

I'm pretty happy with what I think is a good solution here: #887

@kopepasah

This comment has been minimized.

@westonruter perhaps we should refrain from inserting comments, as they contribute to the 50kb limit.

This comment has been minimized.

Copy link
Member Author

replied Jan 19, 2018

Yes. My thought here is that the Customizer could use the comment as a way to inject Custom CSS when modified. At least we can prevent showing this if not is_customize_preview().

@kopepasah

This comment has been minimized.

@westonruter styles inserted should be compressed to remove white space, as it contributes to the 50kb limit.

This comment has been minimized.

Copy link
Member Author

replied Jan 19, 2018

What whitespace is present?

This comment has been minimized.

Copy link
Member Author

replied Jan 19, 2018

Or just generally it should be compressed? See #688. In general I think that it should be up to the plugin to have a build step to minify CSS. That being said, if we do implement the logic to parse CSS rules out to strip what isn't actually used, then we can potentially do a degree of minification as well.

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.