-
Notifications
You must be signed in to change notification settings - Fork 8
Recursive sort #50
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
Recursive sort #50
Conversation
Here are the benchmarks with the additional nest feature. My setup: // postcss.config.js
module.exports = {
map: false,
plugins: [
// Optimizations
require("postcss-preset-env")({
stage: 4,
features: {
"nesting-rules": { noIsPseudoSelector: true },
},
minimumVendorImplementations: 3,
autoprefixer: false,
}),
require("cssnano")(nano),
require("postcss-merge-selectors")({}),
require("postcss-sort-media-queries")({
sort: "desktop-first",
onlyTopLevel: true,
mergeAtRules: false,
nested: false,
configuration: {
unitlessMqAlwaysFirst: true,
},
}),
require("postcss-precision")({}),
],
}; |
Hi, @vis97c I think it is necessary to divide this functionality into 2 different plugins because additional complexity is added with different types of atRules. Nedd new |
Fair enough. I will split the code then. |
@yunusga Do you think this module should stick to sorting without actually merging? Because with the second module that functionality will be duplicated if we take into account the current behavior of the plugin. Merge & sort. I'm thinking for this module: Sorting recursively but do not merge. So the usage will look like this: // postcss.config.js
module.exports = {
map: false,
plugins: [
// Sort media querie like at rules (@media, @container)
require("postcss-sort-media-queries")({
sort: "desktop-first",
onlyTopLevel: true,
mqRegex: /(media|container)/im, // under consideration as well
configuration: {
unitlessMqAlwaysFirst: true,
},
}),
// Merge at rules, preserving the previous order
require("postcss-merge-at-rules")({
atRuleRegex: /(media|container|layer|scope|supports)/im,
nest: true,
}),
],
}; |
@yunusga as promised, I just created the package: postcss-merge-at-rules. Don't worry I will be also be updating this merge request with the propper changes (recursive sorting mostly). I hope we could get to an agreement on the before mention concerns before that. |
Hi @vis97c, sorry, there was a lot of work, I will try to solve the issue with the update today or tomorrow |
Following the disscusions on #38 and OlehDutchenko/sort-css-media-queries#24 I decided to implement some sort of feature to adress it. Feel free to comment and let me know what is needed for this to be merged. The code is not complete (missing tests coverage) but it works. I wanted to open the PR because I was also working on an aditional feature to nest the media queries conditionally.
In implementing the changes mainly because I'm trying to ship the smallest possible css using stable enough features. Merging the atRules and media queries has reduced 30% of the file size already and I'm pushing for another significant reduction after I'm able to implement the media querie nesting. I'm having problems rn but I've aready ran out of time to figure it out, so probably next week I'll work on it.
Again I'll await your comments.