-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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] Move stylesheet generator to extendTheme
#41446
[system] Move stylesheet generator to extendTheme
#41446
Conversation
…rial-ui/extend-theme-zero
…rial-ui/extend-theme-zero
…rial-ui/extend-theme-zero
Netlify deploy previewhttps://deploy-preview-41446--material-ui.netlify.app/ @material-ui/core: parsed: +0.15% , gzip: +0.13% Bundle size reportDetails of bundle changes (Toolpad) |
apps/pigment-next-app/next.config.js
Outdated
getSelector: function getSelector(colorScheme, css) { | ||
if (colorScheme && colorScheme === 'dark') { | ||
return { | ||
'@media (prefers-color-scheme: dark)': { | ||
':root': css, | ||
}, | ||
}; | ||
} | ||
return ':root'; | ||
}, |
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 default extendTheme
use data-*
attribute for creating selectors. This custom getSelector proves that it can be customized.
…rial-ui/extend-theme-zero
…rial-ui/extend-theme-zero
…rial-ui/extend-theme-zero
Signed-off-by: Siriwat K <[email protected]>
Signed-off-by: Siriwat K <[email protected]>
Signed-off-by: Siriwat K <[email protected]>
…rial-ui/extend-theme-zero
@@ -2,34 +2,8 @@ import { serializeStyles } from '@emotion/serialize'; | |||
import { Theme } from './extendTheme'; | |||
|
|||
export function generateTokenCss(theme: Theme) { |
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.
Can you also add tests for these files now that the testing is set-up.
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.
Done in 0bba432. Thanks for the request, I am able to prevent the error with the tests if theme is not created by extendTheme()
I will move this to |
themeProp.defaultColorScheme = resolvedDefaultColorScheme; | ||
themeProp.colorSchemeSelector = colorSchemeSelector; | ||
themeProp.attribute = attribute; |
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.
Is this normal? We are violating the immutability principal. Can we remove/deprecate this? At the minimum, I think that there must be a comment in the code to explain why for the next wave of thousand of developers who will read the source and be confused.
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.
Thanks for pointing out. It needs to be cleanup, I will open a separate PR.
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.
Great thanks. If it's expected, a comment would be great to help understand.
A part of #40225
closes #40178
Summary
extendTheme
for all packages (Material UI and next, Joy UI, Pigment CSS) viagenerateStyleSheets()
.generateCssVars
is refactored togenerateThemeVars
to remove the duplicated logic across packages.@media prefers-color-scheme
, classes, or data attributes can be fully controlled bygetSelector
Next step
attribute
,colorSchemeSelector
props from CssVarsProvider (configured through theme instead)attribute
torootSelector
.