-
Notifications
You must be signed in to change notification settings - Fork 384
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 AMP support for Twenty Nineteen #1587
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.
Approved
Hi @westonruter,
This looks good, and validation of the site with Twenty Nineteen shows no validation error.
Before
After
@@ -141,7 +141,8 @@ public function render_theme_support() { | |||
$theme_support = AMP_Options_Manager::get_option( 'theme_support' ); | |||
$paired_description = __( 'Reuses active theme\'s templates to display AMP responses, but uses separate URLs for AMP. The canonical URLs for your site will not have AMP. If there are AMP validation errors encountered in the AMP response and the validation errors are not accepted for sanitization, then the AMP version will redirect to the non-AMP version.', 'amp' ); | |||
$native_description = __( 'Reuses active theme\'s templates to display AMP responses but does not use separate URLs for AMP. Your canonical URLs are AMP. AMP-specific blocks are available for inserting into content. Any AMP validation errors are automatically sanitized.', 'amp' ); | |||
$builtin_support = in_array( get_template(), array( 'twentyfifteen', 'twentysixteen', 'twentyseventeen' ), true ); | |||
|
|||
$builtin_support = in_array( get_template(), AMP_Core_Theme_Sanitizer::get_supported_themes(), true ); |
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.
Good idea to abstract this into a method.
3ae13a7
to
e09c826
Compare
There are outstanding things todo here, including:
focusable
attribute: ✨ Allow SVG 'focusable' attribute from SVG Tiny 1.2 amphtml#19128@charset
: Strip @charset CSS at-rules #1515@page
(optional: can be done as part of Update AMP spec to 757 (v1811091519050) #1588).Fixes #1515.