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

CSS in Reader mode is not minified #688

Closed
paulschreiber opened this issue Apr 24, 2017 · 7 comments · Fixed by #1048 or #3781
Closed

CSS in Reader mode is not minified #688

paulschreiber opened this issue Apr 24, 2017 · 7 comments · Fixed by #1048 or #3781
Assignees
Labels
Enhancement New feature or improvement of an existing one P0 High priority QA passed Has passed QA and is done Release Task Tasks which do not involve engineering
Milestone

Comments

@paulschreiber
Copy link
Contributor

The AMP CSS inserting by the plugin (.alignright through .back-to-top) is not minified.
screen shot 2017-04-24 at 4 48 15 pm

@mjangda
Copy link
Contributor

mjangda commented Apr 28, 2017

We don't minify anything right now. @amedina has some code in #668 for minification that we can probably generalize and re-use for this.

@amedina amedina self-assigned this Jun 2, 2017
@amedina
Copy link
Member

amedina commented Jun 2, 2017

Yes; we should do this. Will try to get to it asap.

@amedina amedina added P0 High priority Enhancement New feature or improvement of an existing one Task Tasks which do not involve engineering labels Aug 11, 2017
@westonruter
Copy link
Member

Minifying CSS with PHP can lead to unexpected results, especially considering that the content property can reference any string at all. Maybe the occurrence of such problems would be rare, but in order to do it properly we'd need to use an actual CSS tokenizer rather than use regular expressions for search and replace.

@paulschreiber
Copy link
Contributor Author

It should be minified with an existing tool, such as cssmin. Don't write your own minifier. If this CSS is static, it can be done at build time, not runtime.

@westonruter
Copy link
Member

CSS in Classic/Reader mode is still not minified. See #2202 for how we can extend the minification and tree shaking to this mode.

@westonruter westonruter reopened this Apr 26, 2019
@westonruter westonruter changed the title AMP CSS not minified CSS in Reader mode is not minified Jul 30, 2019
@kienstra
Copy link
Contributor

kienstra commented Feb 4, 2020

Steps To Test

  1. Select Reader mode
  2. Create a new post with any title or content
  3. Go to the front-end of the AMP URL, and inspect the source
  4. Expected: the <style amp-custom> is minified. There shouldn't be large spaces inside the style rules.

style-amp

@csossi
Copy link

csossi commented Feb 4, 2020

Verified in QA

@csossi csossi added the QA passed Has passed QA and is done label Feb 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one P0 High priority QA passed Has passed QA and is done Release Task Tasks which do not involve engineering
Projects
None yet
8 participants