Skip to content

Comments

Print amp-default.css via amp_post_template_css action instead of in style.php template#5282

Merged
westonruter merged 3 commits intodevelopfrom
fix/ensuring-legacy-amp-default-styles
Aug 27, 2020
Merged

Print amp-default.css via amp_post_template_css action instead of in style.php template#5282
westonruter merged 3 commits intodevelopfrom
fix/ensuring-legacy-amp-default-styles

Conversation

@westonruter
Copy link
Member

Summary

Fixes https://wordpress.org/support/topic/no-photos-showing-since-update-2-0/

When a theme has customized their style.php template so that it does not include the manual inclusion of amp-default.css, the result is changes to that stylesheet will not be included. So if someone has a Gallery block that is showing as a slideshow, it won't render properly if this CSS from amp-default.css is not added to the page:

.wp-block-gallery[data-amp-carousel="true"] {
display: block;
flex-wrap: unset;
}

If the theme does not intentionally include amp-default.css like so:

<?php echo file_get_contents( AMP__DIR__ . '/assets/css/amp-default.css' ); // phpcs:ignore WordPress.WP.AlternativeFunctions ?>

Then they won't get such rules.

We can avoid this from happening in the future by printing amp-default.css via the amp_post_template_css action, rather than manually printing it in the style.php.

This workaround code also does the trick:

add_action( 'amp_post_template_css', function () {
	echo file_get_contents( AMP__DIR__ . '/assets/css/amp-default.css' ); // phpcs:ignore WordPress.WP.AlternativeFunctions
} );

Checklist

  • My pull request is addressing an open issue (please create one otherwise).
  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added this to the v2.0.1 milestone Aug 27, 2020
@google-cla google-cla bot added the cla: yes Signed the Google CLA label Aug 27, 2020
@westonruter westonruter requested a review from pierlon August 27, 2020 05:45
@westonruter westonruter added Bug Something isn't working CSS Reader Mode WS:Core Work stream for Plugin core labels Aug 27, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Aug 27, 2020

Plugin builds for ce230e6 are ready 🛎️!

@westonruter
Copy link
Member Author

Got a change to improve how this is done…

@westonruter westonruter requested a review from pierlon August 27, 2020 06:03
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Nicely done 👌.

@westonruter
Copy link
Member Author

For the sake of completeness, I'm adding @schlessera and @johnwatkins0 as reviewers in case they can think of any other implications of making this change, given that we would send this straight out without an RC.

@westonruter
Copy link
Member Author

The main implication for a site that is using the unmodified AMP Legacy templates is that the style rules are printed earlier:

image

Since the style rules in amp-default.css are less-specific than the styles in styles.php, this shouldn't make any difference.

@westonruter westonruter merged commit 240bdd8 into develop Aug 27, 2020
@westonruter westonruter deleted the fix/ensuring-legacy-amp-default-styles branch August 27, 2020 19:45
westonruter added a commit that referenced this pull request Aug 27, 2020
@westonruter
Copy link
Member Author

@westonruter
Copy link
Member Author

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA CSS Reader Mode WS:Core Work stream for Plugin core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants