Skip to content
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

[system] getColorSchemeSelector() move away from data-color-scheme "attribute value" selector #42887

Closed
oliviertassinari opened this issue Feb 12, 2024 · 5 comments · Fixed by #42839

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2024

Steps to reproduce

See https://youtu.be/nWcexTnvIKI?si=akGrAE59cm7py1Sl&t=1223

for why this might not be great:

theme.getColorSchemeSelector = (colorScheme: SupportedColorScheme) =>
colorScheme === 'light'
? '&'
: `&[data-joy-color-scheme="${colorScheme}"], [data-joy-color-scheme="${colorScheme}"] &`;

Current behavior

Selector use data attribute value

Expected behavior

Selector use data attribute or class name

Context

For comparison:

Your environment

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Search keywords: data-color-scheme

Search keywords:

Search keywords:

@oliviertassinari oliviertassinari added performance breaking change package: system Specific to @mui/system status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 12, 2024
@oliviertassinari oliviertassinari changed the title [system] getColorSchemeSelector() use class name over data-color-scheme [system] getColorSchemeSelector() move away from data-color-scheme "attribute value" selector Feb 12, 2024
@siriwatknp
Copy link
Member

Noted. Will do some benchmark between [data-color-scheme="dark"] and [data-color-scheme-dark]

@siriwatknp siriwatknp self-assigned this Feb 13, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Feb 13, 2024

I would recommend watching the whole video. There are interesting parts:

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 8, 2024

I'm transferring this to Pigment CSS repository. While there is still discussion about either MUI System and Pigment CSS should be the same thing, or separate (in which case define if it's in Material UI or in Pigment CSS product), for dark mode there isn't much we can do, Pigment CSS needs to be aware.

@oliviertassinari oliviertassinari transferred this issue from mui/material-ui Jun 8, 2024
@siriwatknp siriwatknp transferred this issue from mui/pigment-css Jul 8, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Jul 8, 2024

I'm transferring this to Pigment CSS repository. While there is still discussion about either MUI System and Pigment CSS should be the same thing, or separate (in which case define if it's in Material UI or in Pigment CSS product), for dark mode there isn't much we can do, Pigment CSS needs to be aware.

I'm transferring back to the core repo because it's MUI System related and it will be closed by #42839

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 19, 2024

I'm transferring back to the core repo because it's MUI System related

@siriwatknp Ok, maybe this API is indeed too opinionated for Pigment CSS. MUI System, or whoever we end up calling it, would be its sweet spot.

What I want us to moved toward is for it to definitely not be exported from @mui/material, the more steps we make toward making that package only host CSS, the better:

-import { CssVarsProvider, extendTheme } from '@mui/material/styles'; 
+import { CssVarsProvider, extendTheme } from '@mui/system/CssVarsProvider';
+import { mdTheme } from '@mui/material/mdTheme';

 const theme = extendTheme({
+  ...mdTheme,
   colorSchemes: { dark: true },
 });

 function App() {
   return <CssVarsProvider theme={theme}>{/* ...you app */}</CssVarsProvider>;
 } 

Material UI v6 could have been this opportunity, but we can make it v7, another small iteration change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants