mui / material-ui

Material UI: Ready-to-use foundational React components, free forever. It includes Material UI, which implements Google's Material Design.
https://mui.com/material-ui/
MIT License
91.86k stars 31.57k forks source link

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

Open brijeshb42 opened 2 weeks ago

brijeshb42 commented 2 weeks ago

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

Fixes #42025

mui-bot commented 2 weeks ago

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

brijeshb42 commented 2 weeks ago

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.

aarongarciah commented 2 weeks ago

@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
brijeshb42 commented 1 week ago

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

aarongarciah commented 1 week ago

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

brijeshb42 commented 1 week ago

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.

aarongarciah commented 1 week ago

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.

brijeshb42 commented 1 week ago

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 commented 1 week ago

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