[theme] Improve default theme dark colors
Closed this issue · 12 comments
- The issue is present in the latest release.
- I have searched the issues of this repository and believe that this is not a duplicate.
Current Behavior 😯
The primary and secondary colors are identical between the light and dark theme.
Expected Behavior 🤔
The color should becomes less saturated as in the demos of the documentation and as specified by the Material Design specification.
Steps to Reproduce 🕹
https://codesandbox.io/s/material-ui-dark-theme-41nm4
Your Environment 🌎
| Tech | Version |
|---|---|
| Material-UI | v4.7.2 |
| React | 16.12.0 |
| Browser | Chrome Version 78.0.3904.108 (Official Build) (64-bit) |
The link inherits its color from the parent elements. You need to make sure the color change. You have a couple of options. You should be able to find a more detailed answer on StackOverflow or in this bug & feature tracker (I believe it's a duplicate).
I couldn't find the duplicated issue. So, you have the options between (by order of preference):
- Using CssBaseline component
- Using a wrapping Typography component with component div
- Setting the color prop on your very Link component
@oliviertassinari If you take care to follow the link I put in Steps to Reproduce, you will see that using CssBaseline doesn't help. I put a bare minimum example at that link, that uses just ThemeProvider, CssBaseline and Link. Therefore I don't have any parents which may interfere with the default styling. Aren't dark theme style supposed to apply to Link as well?
Oh, I see, then it's about picking a color with enough contrast :)
What do you think of the following?
https://material.io/design/color/dark-theme.html
diff --git a/packages/material-ui/src/styles/createPalette.js b/packages/material-ui/src/styles/createPalette.js
index 4df6b5efc..b89dd0c19 100644
--- a/packages/material-ui/src/styles/createPalette.js
+++ b/packages/material-ui/src/styles/createPalette.js
@@ -79,16 +79,8 @@ function addLightOrDark(intent, direction, shade, tonalOffset) {
export default function createPalette(palette) {
const {
- primary = {
- light: indigo[300],
- main: indigo[500],
- dark: indigo[700],
- },
- secondary = {
- light: pink.A200,
- main: pink.A400,
- dark: pink.A700,
- },
+ primary: primaryOption,
+ secondary: secondaryOption,
error = {
light: red[300],
main: red[500],
@@ -100,6 +92,32 @@ export default function createPalette(palette) {
...other
} = palette;
+ const primary =
+ primaryOption || type === 'light'
+ ? {
+ light: indigo[300],
+ main: indigo[500],
+ dark: indigo[700],
+ }
+ : {
+ light: indigo[100],
+ main: indigo[200],
+ dark: indigo[300],
+ };
+
+ const secondary =
+ secondaryOption || type === 'light'
+ ? {
+ light: pink.A200,
+ main: pink.A400,
+ dark: pink.A700,
+ }
+ : {
+ light: pink[100],
+ main: pink[200],
+ dark: pink[300],
+ };
+
// Use the same logic as
// Bootstrap: https://github.com/twbs/bootstrap/blob/1d6e3710dd447de1a200f29e8fa521f8a0908f70/scss/_functions.scss#L59
// and material-components-web https://github.com/material-components/material-components-web/blob/ac46b8863c4dab9fc22c4c662dc6bd1b65dd652f/packages/mdc-theme/_functions.scss#L54Ran into the above stated issue of the Link object not respecting light and dark variants defined in the theme. Started on a PR for a possible fix.
@Davst what do you think of #19105 (comment)
@oliviertassinari Ah yeah to clarify, I ran into part of this issue. In my case being that if you have manually defined a light: and dark: value for the primary or secondary color, the Link object will still just grab the main color.
The discussion you linked looks good but I realize that my issue might be better described in: #18831 (comment)
But that ticket was closed, so this was the closest I could find to the same issue.
For clarity what I was planning on making a quick fix for was that Typography doesn't respect if you manually define light: and dark: variants to the primary or secondary color. This means that Link won't use your specified dark or light variants either, the problem as i see it lies in the following line:
https://github.com/mui-org/material-ui/blob/master/packages/material-ui/src/Typography/Typography.js#L81
This and the secondary should be updated to check if there is a variant that corresponds to the type
The original ticket description outlines the same problem, some of the discussion on this issue has evolved beyond the main description to involve systematic approaches to color systems which is nice as well. At the current time, I just really need to fix the fact that Link and Typograpåhy ignores my defined colors.
If you want to separate the two issues, I could fix this to #18831 (comment) if you want to reopen that or something (not sure of the exact etiquette here :) )
the Link object will still just grab the main color.
@Davst I think that the component should be the less dependent on the light or dark mode as possible, we are using color.main on purpose, I don't think that we should change it. The light, main, and dark keys are meant to be used at the same time. So yes, it's the expected behavior, if you need something different, you can change the main value right when you set the dark modality.
@oliviertassinari really? Say I use a setting to check for dark mode via system settings and show up with the light theme on a computer with light mode, but with the dark theme on their phone using dark mode.
Am I to understand that the expected behavior would be to render links possibly unreadable in one of the situations by using the main color which in my case works nicely for light but is garbage for dark mode instead of conforming to how the rest of the theme creation works where I can define the correct color to use for dark mode?
I must be missing something in how you mean to define the colors. To me this seems very odd and inconsistent with how themes are structured for the majority of other objects.
Could you provide an example of how you'd suggest to provide a user selectable, or system-setting-based dark/light mode toggle toggle that would allow to adapt the primary and secondary color?
Also, wouldn't changing the main value when you set the modailty create problems with packaging the theme?
@Davst You can check how the documentation website solves this problem.
https://github.com/mui-org/material-ui/blob/e5b2e229b5ed4366ba89d47c9a4295d234a74d7e/docs/src/modules/components/ThemeContext.js#L181-L204
Ok, thanks, I can use that.
Maybe an issue should be raised regarding the documentation website since as far as I understand from https://material-ui.com/customization/palette/ it indicates the usage I was looking to fix, which might be the source of people (and me) missunderstanding this.
This example:
import { createMuiTheme } from '@material-ui/core/styles';
const theme = createMuiTheme({
palette: {
primary: {
// light: will be calculated from palette.primary.main,
main: '#ff4400',
// dark: will be calculated from palette.primary.main,
// contrastText: will be calculated to contrast with palette.primary.main
},
secondary: {
light: '#0066ff',
main: '#0044ff',
// dark: will be calculated from palette.secondary.main,
contrastText: '#ffcc00',
},
// Used by `getContrastText()` to maximize the contrast between
// the background and the text.
contrastThreshold: 3,
// Used by the functions below to shift a color's luminance by approximately
// two indexes within its tonal palette.
// E.g., shift from Red 500 to Red 300 or Red 700.
tonalOffset: 0.2,
},
});