[Button] Fix contained with inherit prop not adapting on dark mode #34508
Conversation
Netlify deploy previewhttps://deploy-preview-34508--material-ui.netlify.app/ @material-ui/core: parsed: +0.08% , gzip: +0.05% Bundle size report |
michaldudak
added
design: material
|
@danilo-leal @mnajdova could you guys take a look at this please? |
|
I will take a look at this tomorrow! thanks for the fix. |
|
Thanks for looking into this. I would probably make the background a bit lighter in light mode. Also, could you please add a regression test for this? Using the codsandbox linked in the issue can be a great start: https://codesandbox.io/s/togglecolormode-material-demo-forked-1j544?file=/demo.js You can add the fixture here: https://github.com/mui/material-ui/tree/master/test/regressions/fixtures/Button |
There was a problem hiding this comment.
Looks like these changes affect both light and dark modes, should this be improved in v6? @mnajdova
There was a problem hiding this comment.
Echoing @mnajdova's comment, I'd probably make the light background color lighter and the dark mode bg darker as well.
Good point, maybe we should focus on changing only dark mode in this PR. |
feliperli
force-pushed
the
fix/29749/contained-inherit-button-dark-mode
branch
from
f79e94f to
fcea75f
Compare
feliperli
force-pushed
the
fix/29749/contained-inherit-button-dark-mode
branch
from
37c5b9d to
a035ca1
Compare
|
Hey @siriwatknp @mnajdova @danilo-leal , I'm Jessica's friend and will be dealing with this PR for now on :) I've added the regression test based on
Mention me for futher feedback :) Thanks! |
| @@ -121,7 +121,10 @@ const ButtonRoot = styled(ButtonBase, { | |||
| }, | |||
| }), | |||
| ...(ownerState.variant === 'contained' && { | |||
| backgroundColor: (theme.vars || theme).palette.grey.A100, | |||
| backgroundColor: | |||
| (theme.vars || theme).palette.mode === 'dark' | |||
There was a problem hiding this comment.
When using CSS variables, ideally we would use the same variable for both dark and light mode, and we would only switch the value of the variable. cc @siriwatknp should we add a variable for the inherit background of the button?
There was a problem hiding this comment.
@mnajdova @siriwatknp should we use this approach here? or is it good to go as it is?
There was a problem hiding this comment.
I would tend to use CSS variables, for e.g. as it is done for other components: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/styles/experimental_extendTheme.js#L137. cc @siriwatknp
ZeeshanTamboli
changed the title
ZeeshanTamboli
changed the title
There was a problem hiding this comment.
I continued on this PR and made the remaining changes.
I added the CSS vars for Button in extend theme as pointed out by @mnajdova in #34508 (comment).
I tested and it works well both with and without CssVarsProvider.
- Without CssVarsProvider - https://codesandbox.io/s/togglecolormode-material-demo-forked-1dlsye?file=/demo.js
- With CssVarsProvider - https://codesandbox.io/s/togglecolormode-material-demo-forked-99h1p7?file=/demo.js
Also, as @siriwatknp pointed out that it is a breaking change in light mode in #34508 (review), I kept the light mode values same and added new ones for dark mode. The related issue is also specifically only for dark mode.
I also improved the visual regression test to test it in both light and dark palette modes.
It looks good to me to merge. However, it needs one other final review.
co-authored: @EduardoSCosta
This PR resolve #29749