Skip to content

Add initial canonical AMP support#857

Merged
westonruter merged 9 commits intodevelopfrom
add/canonical-head
Jan 12, 2018
Merged

Add initial canonical AMP support#857
westonruter merged 9 commits intodevelopfrom
add/canonical-head

Conversation

@westonruter
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@ThierryA
Copy link
Copy Markdown
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