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 AMP support for Twenty Nineteen #1587

Merged
merged 8 commits into from Nov 15, 2018
Merged

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Nov 3, 2018

There are outstanding things todo here, including:

Fixes #1515.

Copy link
Contributor

@kienstra kienstra left a 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

before-core-sanitizer-update

After

after-core-sanitizer-update

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

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.

@westonruter westonruter changed the title [WIP] Add initial AMP support for Twenty Nineteen Add initial AMP support for Twenty Nineteen Nov 15, 2018
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.

None yet

2 participants