Skip to content
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

Unchanged colours on theme switching (dark/light) #18831

Closed
2 tasks done
aress31 opened this issue Dec 14, 2019 · 14 comments
Closed
2 tasks done

Unchanged colours on theme switching (dark/light) #18831

aress31 opened this issue Dec 14, 2019 · 14 comments
Labels
support: Stack Overflow Please ask the community on Stack Overflow

Comments

@aress31
Copy link

aress31 commented Dec 14, 2019

The palette colours (primary and secondary colours) remain unchanged over dynamically switching from dark/light themes. This is behaviour can clearly be observed with <Typography> objects, this forces developers to programmatically check if the theme is in dark or light mode and to accordingly adapt the colours adding a lot of overhead.

  • 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 😯

Expected Behavior 🤔

I defined my primary colour to red (import red from '@material-ui/core/colors/red';). On a dark mode in the colour code is #f44336 and on a light mode the colour code is #f44336. See the following screenshots:

Dark mode:
image

Light mode:
image

The theme should automatically switch colours according to the selected mode, in dark mode a lighter variant (e.g. red[300]) of colours should be picked whereas in light mode darker variant of the palette colour should be picked (e.g. red[700]) to increase the contrast and improve the overall design.

Steps to Reproduce 🕹

Steps:

  1. Set a colour palette, e.g. primary colour to red and secondary colour to yellow.
  2. Display a text using the <Typography> component with the color:'primary' propriety set.
  3. Open Google Chrome/Firefox developer tool and compare the hex colour code of the generated text on dark and light mode to notice that it is unchanged.

Context 🔦

I am trying to maximise the contrast of my text elements and improve the overall aesthetic of my website.

Your Environment 🌎

Tech Version
Material-UI v4.7.2
React v16.12.0
Browser Google Chrome v78.0.3904.108
TypeScript No
etc.
@oliviertassinari oliviertassinari added the support: Stack Overflow Please ask the community on Stack Overflow label Dec 14, 2019
@support
Copy link

support bot commented Dec 14, 2019

👋 Thanks for using Material-UI!

We use GitHub issues exclusively as a bug and feature requests tracker, however,
this issue appears to be a support request.

For support, please check out https://material-ui.com/getting-started/support/. Thanks!

If you have a question on StackOverflow, you are welcome to link to it here, it might help others.
If your issue is subsequently confirmed as a bug, and the report follows the issue template, it can be reopened.

@support support bot closed this as completed Dec 14, 2019
@oliviertassinari
Copy link
Member

It's somewhat related to #18776.

@aress31
Copy link
Author

aress31 commented Dec 14, 2019

Yes, it is indeed kind of related, I believe that an effort should be made to ensure that the colour property of all components get updated on theme switching. In the case that I documented in this ticket, the bug affects <Typography>, #18776 relates to <Link> but this problem could also very well affect ` and other components.

The fix is quite simple, just switch the colour property of non-surface objects to (primary/secondary).light on a dark theme and (primary/secondary).dark on a light theme this is what I have been programmatically doing on the previous website I designed with Material UI and having this issue fixed on the library level would be awesome!

@oliviertassinari
Copy link
Member

@aress31 Without a reproduction, I'm not sure to understand the problem you are facing. As far as I know, it's a documentation/teaching issue.

@aress31
Copy link
Author

aress31 commented Dec 14, 2019

Alright, let me try to rephrase the bug/issue.

The color propriety of ALL components should automatically get updated when proving a new theme to MuiThemeProvider, i.e. on theme switching (dark/light). In such a way that if I set a Foobar with the following palette:

palette: {
      type: isDarkMode ? 'dark' : 'light',
      primary: red,
      secondary: grey,
      // Used by `getContrastText()` to maximize the contrast between the background and
      // the text.
      contrastThreshold: 4,
      // Used 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.4,
      background: {
        default: isDarkMode ? '#121212' : '#FFFFFF',
      },
    },

The colour of the <Typography> component (see the above) should be set to palette.primary.light when isDarkMode === true and to palette.primary.dark when isDarkMode === false which is currently not the case.

@aress31 aress31 changed the title Unchanged colours over theme switching (dark/light) Unchanged colours on theme switching (dark/light) Dec 14, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 14, 2019

It wasn't designed to behave this way, you need to change the colors.

@aress31
Copy link
Author

aress31 commented Dec 14, 2019

Ok @oliviertassinari understood, but maybe this should be considered as it helps boosting up the contrast between foreground and background, or at least maybe the option to have the colour palette behave this way should be implemented?

@oliviertassinari
Copy link
Member

@aress31 I believe it's the responsibility of the developers when providing a custom palette to make sure the contrast is good enough.

@aress31
Copy link
Author

aress31 commented Dec 15, 2019

@oliviertassinari I totally agree but the problem here is that only the main colour is being used in <Typography> regardless of the theme type (light and dark).

@aress31
Copy link
Author

aress31 commented Dec 26, 2019

@oliviertassinari, I think that my issue was not entirely understood.

Material Design states that on a dark theme, unsaturated colours should be used in order to improve the contrast with the background and limit eye strains as opposed to a light theme where saturated colours are used.

Right now MuiTheme takes a main, a light and a dark colour variant, however, elements such as <Typography> only stick to the main colour variant regardless of the theme value. This seems to be a bug to me, as if following the Material Design doc, on a dark theme the light colour variant should automatically be applied to all display elements.

I hope that I made my ticket clearer. 😁

@luminaxster
Copy link
Contributor

@aress31 I faced the same issue after is stopped using the jss provider. In short, rather than passing the theme you generated once for ThemeProvider, you have to generate it each time you change it so it affects all styles. In code:

const themeA =createMuiTheme(...);  --> const geThemeA =()=>createMuiTheme(...);
const themeB =createMuiTheme(...); --> const getThemeB =()=>createMuiTheme(...);
...
switchTheme=(...)...setState{theme:a?themeA:themeB}; --> switchTheme(...)...setState{theme:a?geThemeA():geThemeB()};
...
<ThemeProvider theme={theme}>

@tantra-anthony
Copy link

tantra-anthony commented Jun 26, 2020

@aress31 I faced the same issue after is stopped using the jss provider. In short, rather than passing the theme you generated once for ThemeProvider, you have to generate it each time you change it so it affects all styles. In code:

const themeA =createMuiTheme(...);  --> const geThemeA =()=>createMuiTheme(...);
const themeB =createMuiTheme(...); --> const getThemeB =()=>createMuiTheme(...);
...
switchTheme=(...)...setState{theme:a?themeA:themeB}; --> switchTheme(...)...setState{theme:a?geThemeA():geThemeB()};
...
<ThemeProvider theme={theme}>

This works for us. The createMuiTheme needs to be instantiated every time a change is made and this also shouldn't be an issue performance wise. Can this be documented or are there any plans on fixing this?

@kishansig10
Copy link

@aress31 I faced the same issue after is stopped using the jss provider. In short, rather than passing the theme you generated once for ThemeProvider, you have to generate it each time you change it so it affects all styles. In code:

const themeA =createMuiTheme(...);  --> const geThemeA =()=>createMuiTheme(...);
const themeB =createMuiTheme(...); --> const getThemeB =()=>createMuiTheme(...);
...
switchTheme=(...)...setState{theme:a?themeA:themeB}; --> switchTheme(...)...setState{theme:a?geThemeA():geThemeB()};
...
<ThemeProvider theme={theme}>

Thanks this helps!! 🚀

@alansouzati
Copy link
Contributor

I wish there was a way in the Box component to use declarative colors and let the framework pick the right one based on the current theme mode. For example:

const aTheme = createTheme({
  palette: {
    appBackground: {
      light: 'gray',
      dark: 'black',
    },
  }
});

<Box bgcolor="appBackground">HI</Box>

if the mode is light, then it uses gray, otherwise if it is dark it uses black.

I also don't full understand why main is required.

As of today I keep having to change my markup with boilerplate repetitive code like this:

<Box  bgColor={isDark ? 'appBackground.dark' : 'appBackground.light'} />

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
support: Stack Overflow Please ask the community on Stack Overflow
Projects
None yet
Development

No branches or pull requests

6 participants