material-ui
material-ui copied to clipboard
[system][enhancement] Dedupe zero min width media queries
from the generated css. These don't have any higher specificity than direct css.
Fixes #42025
- [x] I have followed (at least) the PR section of the contributing guide.
Netlify deploy preview
https://deploy-preview-42064--material-ui.netlify.app/
Bundle size report
Details of bundle changes (Toolpad) Details of bundle changes
Generated by :no_entry_sign: dangerJS against 13096fa41e7d9ca61afe64effd9188bdd9cd0dfd
Seems this change is breaking something as is evident in the argos
screenshot. I'll investigate and see if we need a deeper level of testing before merging it.
@brijeshb42 margins for Stack's children elements in the smallest breakpoint seem to be injected last, so they win. This is from the Responsive values example in System's docs:
before | after |
---|---|
After testing the change, I feel that although this is the correct change to have and will benefit us in Pigment CSS to generate relatively smaller bundle sizes, I'm afraid that we might be breaking sites for a lot of our users who might have unknowingly started to rely on this behaviour. Here's an example from one of our own templates that got captured through argos
-
Before | After |
---|---|
https://next.mui.com/material-ui/getting-started/templates/dashboard/ | https://deploy-preview-42064--material-ui.netlify.app/material-ui/getting-started/templates/dashboard/ |
cc: @siriwatknp
@brijeshb42 I see the problem now. Before this change, users' overrides always came after the component code.
The original code does this (in a minimal pseudo-code representation):
.Toolbar {
padding-left: 16px;
}
@media (min-width: 600px) {
.Toolbar {
padding-left: 24px;
}
}
/* User code 👇 */
@media (min-width: 0px) {
.Toolbar {
padding-left: 8px; /* 🏆 this declaration wins 🟢 */
}
}
With the new changes, the generated code looks like this:
.Toolbar {
padding-left: 16px;
/* User code is mixed with the component's CSS 👇 */
padding-left: 8px;
}
@media (min-width: 600px) {
.Toolbar {
padding-left: 24px; /* 🏆 this declaration wins 🔴 */
}
}
I'm wondering if the solution is to follow the same approach as the original code by appending a new ruleset at the end but without the media query:
.Toolbar {
padding-left: 16px;
}
@media (min-width: 600px) {
.Toolbar {
padding-left: 24px;
}
}
/* User code 👇 */
.Toolbar {
padding-left: 8px; /* 🏆 this declaration would win 🟢 */
}
I'm unaware if this is possible or not, I'm just wondering if this could lead to a potential solution.
It is definitely possible. But in that case, the only gain I see that we are saving this much string (@media (min-width: 0px) {}
) from being added which isn't a lot I'd say.
the only gain I see that we are saving this much string (@media (min-width: 0px) {}) from being added which isn't a lot I'd say.
Yeah, although how many times it's repeated in the generated CSS bundle depends on consumers' code. Which may not be so bad when compression enters the equation. But if we think about the big picture (thousands of CSS files being generated by Pigment CSS in the future), may be worth saving some bytes if the added complexity and risk is low.
I did a basic gzipped size check for this template
First one with zero media query -
fs = require('fs');
a = '';
for (let i=0;i<10000;i++) {
a += `@media (min-width: 0px){.class${i}{padding:10px;}}` + `@media (min-width: ${i}px){.cls${i}{margin: ${i}px;}}`
}
fs.writeFileSync('a1.css', a, 'utf-8')
And second one without media query -
fs = require('fs');
a = '';
for (let i=0;i<10000;i++) {
a += `.class${i}{padding:10px;}` + `@media (min-width: ${i}px){.cls${i}{margin: ${i}px;}}`
}
fs.writeFileSync('a2.css', a, 'utf-8')
There's definitely reduced size without it (~7.4%)
Before:
After:
@aarongarciah I spoke too soon. I tried to implement what you suggested -
.Toolbar {
padding-left: 16px;
}
@media (min-width: 600px) {
.Toolbar {
padding-left: 24px;
}
}
/* User code 👇 */
.Toolbar {
padding-left: 8px; /* 🏆 this declaration would win 🟢 */
}
but there doesn't seem to be a straightforward way to do this as the modification will need to be down outside of the media queries object.
@brijeshb42 I agree with this PR. Have you checked the theme.containerQueries
? It relies on theme.breakpoints
, so I think it should have this logic too.
Also, the breaking changes that you mention, is it still the case? I don't see that failure in Argos.