-
Notifications
You must be signed in to change notification settings - Fork 331
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
Upgrade to CSSNANO v5 #3245
Upgrade to CSSNANO v5 #3245
Conversation
@@ -6,7 +6,11 @@ describe('PostCSS config', () => { | |||
let env | |||
|
|||
function getPluginNames ({ plugins }) { | |||
return plugins.map(({ postcssPlugin }) => postcssPlugin) | |||
return plugins.flatMap(getPluginName) |
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.
The upgrade to cssnano@5
runs as a "processor" with its own list of plugins so we can add these to the list
bbc3a28
to
dba5661
Compare
dba5661
to
67d4a80
Compare
67d4a80
to
b87133a
Compare
Diff for
|
Diff for
|
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.
🧹
This looks grand to me - linting's passing, and at a glance, the diff looks like it's all just rule re-ordering.
Thanks @domoscargin I'll put a nudge out for more 👀 |
As discussed on Slack, the re-ordering is happening because of a new rule in CSS nano which sorts CSS declarations based on their property names so that it gzips better. We already enforce a specific order for properties with our linter, so shouldn't see much of a gain (it's just sorting properties in a different order). I think we should disable the rule because it keeps the dist CSS closer to the authored Sass. As an example of how this benefits us, when reviewing the changes using Chrome dev tools, the properties currently appear in the same order they appear in the Sass, whereas with this change they are sorted alphabetically, which makes it harder to debug (especially where some of the properties are coming from mixins). |
Thanks @36degrees makes perfect sense Diffs updated above Notable things:
|
To explain the duplicate media query removal (diff is hard to read) Before@media (min-width:48.0625em) {
.govuk-header__logo {
width: 33.33%;
padding-right: 15px;
float: left;
vertical-align: top
}
}
@media (min-width:48.0625em) {
.govuk-header__content {
width: 66.66%;
padding-left: 15px;
float: left
}
} After@media (min-width:48.0625em) {
.govuk-header__logo {
width: 33.33%;
padding-right: 15px;
float: left;
vertical-align: top
}
.govuk-header__content {
width: 66.66%;
padding-left: 15px;
float: left
}
} |
Sorted CSS is smaller when gzipped, but we sort using Stylelint
dce768a
to
effef6f
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.
Nice! 🎉
This PR updates
cssnano@4
(plugin) tocssnano@5
(processor)See release notes:
https://cssnano.co/blog/cssnano-5-release-notes/
Full list of now-bundled plugins can be found here:
https://cssnano.co/docs/what-are-optimisations/
What's a PostCSS processor?
Effectively a processor is a bundle of other plugins:
https://postcss.org/api/#processor
Note: The CSS diff shows various rules have been re-ordered again
Compatibility
Previous CSSNANO upgrades had been rejected for two reasons
Test failures
We hadn't spotted the nested
plugins
with CSSNANO as a ProcessorAutoprefixer v10 required (or so we thought)
This is only required when CSSNANO wraps the PostCSS plugins
PostCSS with CSSNANO as plugin (compatible)
PostCSS config with CSSNANO wrapper (incompatible)