-
-
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
[core] Generate vars in extendTheme
#35739
Conversation
Netlify deploy previewhttps://deploy-preview-35739--material-ui.netlify.app/ @material-ui/core: parsed: +0.14% , gzip: +0.18% Bundle size report |
shadowRing: theme.shadowRing, | ||
shadowChannel: theme.shadowChannel, | ||
}; | ||
theme.getColorSchemeSelector = () => '&'; |
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.
The same default implementation as in defaultTheme
.
theme.palette = deepmerge(themeOptions?.palette || {}, { | ||
...(theme.colorSchemes.light.palette as RuntimeColorSystem['palette']), | ||
colorScheme: 'light', | ||
}); |
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 wonder if extendTheme()
should not resolve the palette yet.
My initial intention is that extendTheme()
should return sets of colors for your application but it should not decide what the current color scheme is. That logic will be handled by CssVarsProvider
.
generate `vars` in `extendTheme`
We decided with generating the CSS vars in extendTheme with containing the css variable that defaults to the actual theme css value. |
extendTheme
extendTheme
…Theme/generate-vars
@siriwatknp would be great if you can revalidate the screenshot differences, before I revert the change for adding back the |
Got it, this is next on my list. |
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.
✅ Looks great! pushed a minor fix.
I think this change to unstable_createCssVarsTheme and Provider has messed up custom themes that use |
Can you open a new issue with reproduction? Feel free to tag me. |
@siriwatknp Just saw this, will do! Let me create a minimal reproduction first and I'll make a post! |
Breaking change
shouldSkipGeneratingVar
prop was moved from thecreateCssVarsProvider
's option to thetheme
.extendTheme
from Material UI or Joy UI, it needs to be wrapped insideunstable_createCssVarsTheme
- a util exported from the MUI System.Or define it directly in the theme prop:
As agreed upon in #35446 (comment)
Changes done in the PR:
extendTheme
now adds thetheme.vars
object containing values in a form of:var(--joy-palette-primary-500, #aaaaaa)
- containing default values from the light color schemeCssVarsProvider
now uses theshouldSkipGeneratingVar
from the themeResult:
CssVarsProvider
to be used 🎉 - https://codesandbox.io/s/staging-cache-85f1co?file=/demo.tsxCssVarsProvider
to be used (this was the case before too) - https://codesandbox.io/s/winter-haze-txd52g?file=/demo.tsx