-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
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
[CssVarsProvider] Always generate new css
object
#36853
Conversation
Netlify deploy previewhttps://deploy-preview-36853--material-ui.netlify.app/ Bundle size report |
@@ -38,9 +38,12 @@ function prepareCssVars<T extends DefaultCssVarsTheme, ThemeVars extends Record< | |||
|
|||
const generateCssVars = (colorScheme?: string) => { | |||
if (!colorScheme) { | |||
return { css: rootCss, vars: rootVars }; | |||
return { css: { ...rootCss }, vars: rootVars }; |
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 vars
can stay the same because we don't alter them anywhere.
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 would have preferred a test that actually tests the issue, here we are testing the implementation of the util only.
const [, rerender] = React.useState(false); | ||
React.useEffect(() => { | ||
// Trigger rerender to ensure that the UI does not change after the first render. | ||
// To catch bug like https://github.com/mui/material-ui/issues/36452 | ||
rerender(true); | ||
}, []); |
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 There is a flaw in this visual regression. It produces the correct snapshot if the component renders just once which happens in the visual regression test.
Added the rerender(true) to trigger the second render. This should cover the bug the this PR solves.
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.
Perfect, thanks for looking into this. Alright, let's merge.
closes #36452
Root cause
There is a logic in
createCssVarsProvider
that will delete some fields from thecss
object to not be included in the:root
. Since thecss
is not always a new object, all of the--mui-overlays-*
are removed in the first render and never exist after that.Fix
just need to ensure that the
css
is always a new object.✅ Test added.