-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
Add CSS variables support for Material UI components #32835
Conversation
@material-ui/core: parsed: +1.78% , gzip: +1.05% |
e4eceab
to
82286e5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided not to go with palette.grey.contrast{number} as described in the #32049 (comment) because it is not practical when I tried to apply it to the components (the main problem is the Chip component).
I can still see usage of contrast4
in the Chip
I guess we need to update it?
93fb139
to
989d1ca
Compare
@mnajdova Can you take a look at the PR again?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One instance where TypeScript would have caught some issues :)
}, | ||
...(!theme.vars && { | ||
'&:-webkit-autofill': { | ||
WebkitBoxShadow: theme.palette.mode === 'light' ? null : '0 0 0 100px #266798 inset', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe few more use-cases for the unset
CSS var :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not put them in theme tokens yet. These are not directly palette tokens. There are only 2 components that contain this logic FilledInput
and OutlinedInput
, so I think let's wait for feedback from the community first.
...(theme.vars && { | ||
'&:-webkit-autofill': { | ||
borderTopLeftRadius: 'inherit', | ||
borderTopRightRadius: 'inherit', | ||
}, | ||
[theme.getColorSchemeSelector('dark')]: { | ||
'&:-webkit-autofill': { | ||
WebkitBoxShadow: '0 0 0 100px #266798 inset', | ||
WebkitTextFillColor: '#fff', | ||
caretColor: '#fff', | ||
}, | ||
}, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova This is the logic when CSS variables exist which uses CSS selector when the mode is dark. I think we can move forward with this (it does not cause dark mode flicker) instead of creating 3 new tokens (the more tokens the bigger CSS output).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah that makes sense 👍
@vicasas Would you mind taking a look if the naming makes sense to you? |
Hi! @siriwatknp I find it incredible! What a great job with Css Vars 👏. I have the following doubts:
Everything else looks good (: |
The
Good point. I will update those components in their PRs (which already existed) after this one is merged. |
!ownerState.enableColorOnDark && { | ||
backgroundColor: null, | ||
color: null, | ||
...(theme.vars && { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...(ownerState.color === 'default' && { | ||
'--AppBar-background': ownerState.enableColorOnDark | ||
? theme.vars.palette.AppBar.defaultBg | ||
: joinVars(theme.vars.palette.AppBar.darkBg, theme.vars.palette.AppBar.defaultBg), | ||
'--AppBar-color': ownerState.enableColorOnDark | ||
? theme.vars.palette.text.primary | ||
: joinVars(theme.vars.palette.AppBar.darkColor, theme.vars.palette.text.primary), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mnajdova the case of AppBar is tricky but I managed to make it works.
To prevent dark mode flickers, we need to make sure that the stylesheet does not change between modes. That's why I have to introduce:
// component variables inside AppBar
--AppBar-background
--AppBar-color
// and use it as usual
backgroundColor: 'var(--AppBar-background)',
color: 'var(--AppBar-color)',
and then at the global theme tokens, create --mui-palette-AppBar-darkBg
only for dark mode.
This will allow the AppBar to use the fallback value when it is on light mode without changing the component stylesheet.
...(ownerState.color === 'default' && {
'--AppBar-background': ownerState.enableColorOnDark
? theme.vars.palette.AppBar.defaultBg
: joinVars(theme.vars.palette.AppBar.darkBg, theme.vars.palette.AppBar.defaultBg),
// The result is
'--AppBar-background': 'var(--mui-palette-AppBar-darkBg, var(--mui-palette-AppBar-defaultBg))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is closer to what I had in mind, the only difference that I was thinking about is setting the value of unset
, but not generating the CSS vars is even better 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting the value of unset
Setting the variable to unset
would not work because the variable exists so it won't use the fallback value.
// the background will be transparent
--variable-1: unset;
background-color: var(--variable-1, #fff);
// the background will be white
background-color: var(--variable-1, #fff);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could find one final bug, the rest looks good
Co-authored-by: Marija Najdova <[email protected]>
✅ #32868 is merged.
A part of #32049 as described in #32049 (comment)
The new tokens are added to
experimental_extendTheme.js
. I decided not to go withpalette.grey.contrast{number}
as described in the proposal because it is not practical when I tried to apply it to the components (the main problem is the Chip component).Instead, I extract the color manipulation into the component tokens which is easier to read and customize.
Why new tokens are necessary?
We intend to ship CSS variables support as a feature in v5 (no breaking change) so when developers opt-in to this feature, they should have the SSR flashing issue fixed (that's what we will advertise). However, some components' implementation contains runtime calculations to switch values between modes, so the new tokens are needed to preserve the same behavior.
Naming
I think the token should have the shortest name possible because it makes the DevTool and usage cleaner. Also, the palette already implies that the tokens are color-related so I don't think
bgColor
orborderColor
is necessary. Lastly, the common customization flow would start from seeing the variable from DevTool and then specify the variable in the theme, so I don't think usingbg
(the abbreviation ofbackground
) will cause confusion.Here are the new tokens:
The next step is to use this pattern in the existing PRs.