-
Notifications
You must be signed in to change notification settings - Fork 381
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
Bump amp-custom style size limit from 50000 to 75000 bytes #4313
Conversation
22cfca1
to
90ff57a
Compare
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.
Yes! Excellent to see this land.
@@ -2077,7 +2077,7 @@ public static function render_single_url_error_details( $validation_error, $term | |||
echo wp_kses_post( | |||
sprintf( | |||
/* translators: 1: Documentation URL, 2: Documentation URL, 3: !important */ | |||
__( 'AMP allows you to <a href="%1$s">style your pages using CSS</a> in much the same way as regular HTML pages, however there are some <a href="%2$s">restrictions</a>. Nevertheless, the AMP plugin automatically inlines external stylesheets, transforms %3$s qualifiers, and uses tree shaking to remove the majority of CSS rules that do not apply to the current page. Nevertheless, AMP does have a 50KB limit and tree shaking cannot always reduce the amount of CSS under this limit; when this happens an excessive CSS error will result.', 'amp' ), | |||
__( 'AMP allows you to <a href="%1$s">style your pages using CSS</a> in much the same way as regular HTML pages, however there are some <a href="%2$s">restrictions</a>. Nevertheless, the AMP plugin automatically inlines external stylesheets, transforms %3$s qualifiers, and uses tree shaking to remove the majority of CSS rules that do not apply to the current page. Nevertheless, AMP does have a 75KB limit and tree shaking cannot always reduce the amount of CSS under this limit; when this happens an excessive CSS error will result.', 'amp' ), |
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 Using a placeholder for the limit here would make the string more future-proof, just an idea.
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.
Yes, I thought about that. However, it is extremely unlikely the limit will be changed again. So that's why I didn't bother.
Summary
See ampproject/amphtml#26466 and ampproject/amphtml#26475.
This PR directly patches
class-amp-allowed-tags-generated.php
without updating the entire spec which will include a lot of other changes. This will facilitate a little 1.4.4 release with a big impact.The validator update has been deployed:
Checklist