-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[material-ui][AppBar] Fix inherit color is inconsistent between ThemeProvider and CssVarsProvider #42714
[material-ui][AppBar] Fix inherit color is inconsistent between ThemeProvider and CssVarsProvider #42714
Conversation
Netlify deploy previewhttps://deploy-preview-42714--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
Hey @ZeeshanTamboli, thanks for working on this. I cannot access the Before and After Sandboxes: @siriwatknp may I ask you to review this as well? |
@DiegoAndai Sorry, I didn't realize it was private. Seems like these days CodeSandbox sets them to private by default. I've made it public now, so you should be able to access it. |
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'm not sure I understand the fix. Doesn't inherit
rely on the --AppBar-color
var being used?
'--AppBar-color': 'inherit', |
color: var(--AppBar-color)
is not added, the inherit
color style doesn't work, does it?
Doesn't the transparent
style apply very similar styles, and overrides these?
material-ui/packages/mui-material/src/AppBar/AppBar.js
Lines 150 to 159 in 523811b
props: { color: 'transparent' }, | |
style: { | |
'--AppBar-background': 'transparent', | |
'--AppBar-color': 'inherit', | |
backgroundColor: 'var(--AppBar-background)', | |
color: 'var(--AppBar-color)', | |
...theme.applyStyles('dark', { | |
backgroundImage: 'none', | |
}), | |
}, |
@DiegoAndai It can be a bit confusing, so I'll explain in detail. When you provide a The issue is with the AppBar component's background color when the For Inspecting the before code sandbox, you'll see the AppBar's background color when background-color: var(--AppBar-background); But
background-color: #fff;
background-color: var(--mui-palette-background-paper); So the fix is to avoid using Proof that this is a regression from #32835 which was released in As mentioned in the PR description, there's a separate PR for v5: #42713 due to different code from v6. |
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 @ZeeshanTamboli 💙
…Provider and CssVarsProvider (mui#42714)
Fixes #42379
Before: https://codesandbox.io/p/sandbox/cool-goldstine-kfzyjk?file=%2Fsrc%2FApp.js%3A8%2C7
After: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-z4c46h?file=%2Fsrc%2FApp.tsx%3A5%2C15
PR for v5: #42713
Proof that this is a regression from #32835 which was released in
v5.8.5
:5.8.4
: code5.8.5
: code