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
Merged

Add initial canonical AMP support #857

merged 9 commits into from
Jan 12, 2018

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jan 12, 2018

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

Todo:

  • Add tests.

Copy link
Collaborator

@ThierryA ThierryA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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() {
Copy link
Collaborator

@ThierryA ThierryA Jan 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just not implemented yet.

*/
add_filter( 'show_admin_bar', '__return_false', 100 );

add_action( 'template_redirect', array( __CLASS__, 'start_output_buffering' ) );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point.

@westonruter westonruter merged commit f2b1831 into develop Jan 12, 2018
@westonruter westonruter deleted the add/canonical-head branch January 12, 2018 19:53
@westonruter westonruter added this to the v0.7 milestone Jan 16, 2018
@ThierryA
Copy link
Collaborator

ThierryA commented Jan 16, 2018

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

@westonruter
Copy link
Member Author

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

@ThierryA
Copy link
Collaborator

Great, thanks @westonruter 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants