-
Notifications
You must be signed in to change notification settings - Fork 383
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
Conversation
There was a problem hiding this 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() { |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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' ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good point.
Start output buffering at template_redirect priority 0
…de canonical links
This is just |
Great, thanks @westonruter 👍 |
wp_head
andwp_footer
actions are removed. See Discovery to ensure all relevant WordPress actions/filters are AMP compatible #850.wp_head
.style.css
and Custom CSS are printed in<style amp-custom>
.amp
. See Allow themes to provide alternate templates in “paired mode” #849.For a theme that makes use of these changes, see: xwp/ampnews#17
Todo: