-
-
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
[docs] Remove custom primary & secondary color #26541
Conversation
Details of bundle changes (experimental) @material-ui/core: parsed: +Infinity% , gzip: +Infinity% |
@@ -106,6 +106,7 @@ const useStyles = makeStyles( | |||
/* Isolate the demo with an outline. */ | |||
demoBgOutlined: { | |||
padding: theme.spacing(3), | |||
backgroundColor: theme.palette.background.paper, |
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.
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.
👍 for the direction taken by the outcome. It seems better to render the demos under theme.palette.background.default
.
However, why use the paper design token? Why not apply the same background color to the whole page?
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.
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.
Interesting. I would add that there are different demos variant. We have:
"bg": true
: https://deploy-preview-26541--material-ui.netlify.app/components/stack/#usage"bg": "outlined"
, the default, https://deploy-preview-26541--material-ui.netlify.app/components/checkboxes/#basic-checkboxes"bg": "inline"
, https://deploy-preview-26541--material-ui.netlify.app/components/paper/#variants
I almost feel like "bg": true should be the current "bg": "outlined" and "bg": "outlined" should have the bgcolor of "bg": "inline". Not sure, I'm not in my comfort zone with design 😁
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.
What you are describing looks completely accurate. The patch of the documentation was a short-term focused solution to not rework the default theme.
I would personally do this change last, once we have fixed the core.
Regarding light vs. dark mode color, there is #18776. I don't think that we can use the same colors (primary included) between light and dark mode.
@@ -106,6 +106,7 @@ const useStyles = makeStyles( | |||
/* Isolate the demo with an outline. */ | |||
demoBgOutlined: { | |||
padding: theme.spacing(3), | |||
backgroundColor: theme.palette.background.paper, |
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.
👍 for the direction taken by the outcome. It seems better to render the demos under theme.palette.background.default
.
However, why use the paper design token? Why not apply the same background color to the whole page?
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.
Nice. Even get's rid of links within <code />
segments not meeting AA contrast ratios 👍
might also close #18378
Careful with these statements. GitHub will close the referenced issue if this PR is merged.
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.
Much better!
Agree on using different primary & secondary colors in light and dark mode. 👍 |
@siriwatknp Before we merge this one. What do you think about:
|
closes #22112
https://deploy-preview-26541--material-ui.netlify.app/
After removing custom primary & secondary color, I found this issue in default primary color in dark mode. The color does not pass the AA standard and hard to see (affect all the components).
Other solution
Instead of fixing docs to be default theme, should we take this opportunity to do the opposite?
Solution 1
fix default theme to have same color as the docs (Note that the current docs has different color between light & dark mode)
Solution 2
find the new default color that works with light & dark mode and apply to docs as well.
However, from material design color tool, I don't see a color that pass AA standard for both light & dark mode. the best color that I tried so far is
#1976d2
=blue[700]
https://webaim.org/resources/contrastchecker/?fcolor=1976D2&bcolor=121212I lean toward having different primary & secondary color (same palette but lighter in dark mode) between light and dark mode. https://material.io/design/color/dark-theme.html
related to #18378
cc @mui-org/core