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

[theme] Improve default primary, secondary and error theme palette #26555

Merged
merged 13 commits into from
Jun 7, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 2, 2021

follow up from #26541

close #18378: dark mode following the spec
close #18776: dark mode color contrast
closes #26288: error color contrast in light mode

https://deploy-preview-26555--material-ui.netlify.app/


The goal of this PR is to have default color palette that pass AA standard for both light & dark theme. From the outcome of #26541, it is not possible to have one color that pass AA standard for both light & dark theme. So the color must be different between mode (same shade but different lightness)

https://material.io/design/color/dark-theme.html#ui-application

Primary color

V4 ❗️

  • indigo[500] in default theme does not pass standard in dark mode

Before-Primary

V5 ✅

  • use custom color in the docs blue[700] so that the docs does not change much and the logo looks consistent.
  • adjust primary.dark and primary.light close to the docs

Primary

Secondary color

V4 ❗️

  • pink.A400 in default theme does not pass standard in light & dark mode

Before-Secondary

V5 ✅

  • change to purple[500] because the docs used pink.A400 which does not pass AA standard and the color is too close to error. As far as I tried, I feel purple sit very well with the primary (blue) color.
  • use purple[200] in dark mode similar to primary color

Secondary

Error color

V4 ❗️

  • red[500] does not pass standard in light mode

Before-Error

V5 ✅

  • increase the number to red[700] in light mode

Error

Info color

  • change to cyan[500] (it was blue[500] in v4) to not use the same color as v5 primary color and still informative color.

warning and success remains the same.

Preview of some components

image

image

image

image

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 2, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against ffa7c77

@oliviertassinari
Copy link
Member

Is it closing #18776 too?

@siriwatknp
Copy link
Member Author

Is it closing #18776 too?

Great, 👍

@siriwatknp siriwatknp marked this pull request as ready for review June 3, 2021 01:35
@@ -4,6 +4,8 @@ import { expect } from 'chai';
import { createMount, describeConformanceV5, act, createClientRender } from 'test/utils';
import FormLabel, { formLabelClasses as classes } from '@material-ui/core/FormLabel';
import FormControl, { useFormControl } from '@material-ui/core/FormControl';
import { hexToRgb } from '@material-ui/core/styles';
import defaultTheme from '../styles/defaultTheme';
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use defaultTheme here to remove hard coded color in the test below.

I wonder should defaultTheme be exported as defaultLightTheme from @material-ui/core/styles also?

In my side project I use defaultTheme a lot and it is quite annoying to call createTheme() everytime.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal of this PR is to have default color palette that pass AA standard for both light & dark theme.

Was this not the case before? Listing these instances in a single place might be interesting for people that manually patched them for this purpose. Sounds like they can now remove code only targetted at patching AA ratio.

which will close #18378, #18776

Github will only close one of them. Each referenced issue/pr needs to have its own magic keyword associated

Comment on lines +179 to +181
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1', // warning palette
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1', // info palette
'falls below the WCAG recommended absolute minimum contrast ratio of 3:1', // success palette
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we include the color in the warning? If not then we can follow-up. Otherwise I'd make sure the color is included in the assertion which means we can get rid of the (potentially wrong) code comments.

Copy link
Member Author

@siriwatknp siriwatknp Jun 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 lines was there at the beginning (1 more is added because info is changed to cyan[500], it's sound intentional that we expect the colors to falls below contrast ratio. I was confused at first about this test so I added comments at the end. Any suggestion?

@siriwatknp
Copy link
Member Author

Was this not the case before?

If you mean #26541, it is related but different. This PR fix the default color from the theme where as #26541 remove custom color in the docs.

Listing these instances in a single place might be interesting for people that manually patched them for this purpose

You mean add to documentation?

@eps1lon
Copy link
Member

eps1lon commented Jun 3, 2021

Was this not the case before?

If you mean #26541, it is related but different.

No, I mean the direct thing I responded to:

The goal of this PR is to have default color palette that pass AA standard

In which instances did we not pass AA?

You mean add to documentation?

In the PR in a prominent spot so that you don't have to read through unrelated conversations to figure out what changed.

@siriwatknp
Copy link
Member Author

In which instances did we not pass AA?

Updated the description

  • primary color indigo[500] in dark mode
  • secondary color pink.A400 in light & dark mode
  • error color red[500] in light mode

@oliviertassinari oliviertassinari changed the title Update default primary, secondary and error theme palette [theme] Update default primary, secondary and error theme palette Jun 3, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm updating the PR's description, it fixes #26288 too. Regarding the contrast issue with error. See below, top down:

  1. Colors used in MD guidelines videos
  2. Colors used in the previous version on material design web components
  3. Our new color

Capture d’écran 2021-06-03 à 17 17 04

@oliviertassinari oliviertassinari changed the title [theme] Update default primary, secondary and error theme palette [theme] Improve default primary, secondary and error theme palette Jun 3, 2021
@michaldudak
Copy link
Member

Could we also change the background color of code? The current greenish-brown seems a bit out of place as it doesn't match anything else on page. IMO it doesn't play well with with the cyan when there's an a inside code (even though the contrast ratio is OK, it just doesn't feel right):
image

I propose we use the same background as in pre:
image

or something a bit lighter, like #333944:
image
image

@siriwatknp
Copy link
Member Author

@michaldudak updated! 👍

image

@oliviertassinari

This comment has been minimized.

@siriwatknp

This comment has been minimized.

@siriwatknp siriwatknp merged commit e35bef1 into mui:next Jun 7, 2021
@siriwatknp siriwatknp deleted the new-default-theme-color branch June 7, 2021 10:31
jonenst added a commit to gridsuite/commons-ui that referenced this pull request May 23, 2023
in mui/material-ui#21514 pseudo classes were moved
from root to content, so we no longer need nested selectors and rules need to go on content directly
use Mui-focused instead of :focus

also restore muiv4 treeitem label color rule to get a different color right after a click on an
item. It was also removed in treeitem in mui/material-ui#21514
Note: in v5 primary.main was changed from indigo to blue in mui/material-ui#26555) but let's
go with the new primary colors
Note2: fade renamed to alpha
jonenst added a commit to gridsuite/commons-ui that referenced this pull request May 23, 2023
After a quick look, we didn' customize TreeViewFinder which also uses TreeView/TreeItem
so it shouldn't need the same fixing as ReportViewer.

in mui/material-ui#21514 pseudo classes were moved
from root to content, so we no longer need nested selectors and rules need to go on content directly
use Mui-focused instead of :focus

also restore muiv4 treeitem label color rule to get a different color right after a click on an
item. It was also removed in treeitem in mui/material-ui#21514
Note: in v5 primary.main was changed from indigo to blue in mui/material-ui#26555) but let's
go with the new primary colors
Note2: fade renamed to alpha
EstherDarkish pushed a commit to gridsuite/commons-ui that referenced this pull request Jun 7, 2023
After a quick look, we didn' customize TreeViewFinder which also uses TreeView/TreeItem
so it shouldn't need the same fixing as ReportViewer.

in mui/material-ui#21514 pseudo classes were moved
from root to content, so we no longer need nested selectors and rules need to go on content directly
use Mui-focused instead of :focus

also restore muiv4 treeitem label color rule to get a different color right after a click on an
item. It was also removed in treeitem in mui/material-ui#21514
Note: in v5 primary.main was changed from indigo to blue in mui/material-ui#26555) but let's
go with the new primary colors
Note2: fade renamed to alpha

Co-authored-by: Sylvain Bouzols <sylvain.bouzols@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants