material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[system][enhancement] Dedupe zero min width media queries

Open brijeshb42 opened this issue 9 months ago • 9 comments

from the generated css. These don't have any higher specificity than direct css.

Fixes #42025

brijeshb42 avatar Apr 30 '24 10:04 brijeshb42

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

mui-bot avatar Apr 30 '24 10:04 mui-bot

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 avatar May 01 '24 06:05 brijeshb42

@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
Screenshot 2024-05-01 at 09 51 04 Screenshot 2024-05-01 at 09 51 19

image

aarongarciah avatar May 01 '24 07:05 aarongarciah

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/
Screenshot 2024-05-06 at 2 37 06 PM Screenshot 2024-05-06 at 2 37 31 PM

cc: @siriwatknp

brijeshb42 avatar May 06 '24 09:05 brijeshb42

@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.

aarongarciah avatar May 06 '24 10:05 aarongarciah

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.

brijeshb42 avatar May 06 '24 10:05 brijeshb42

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.

aarongarciah avatar May 06 '24 11:05 aarongarciah

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: Screenshot 2024-05-06 at 6 07 51 PM

After: Screenshot 2024-05-06 at 6 10 26 PM

brijeshb42 avatar May 06 '24 12:05 brijeshb42

@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 avatar May 06 '24 12:05 brijeshb42

@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.

siriwatknp avatar Jun 09 '24 07:06 siriwatknp