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] Update default info success warning color #26817

Merged
merged 49 commits into from Jul 9, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 18, 2021

BREAKING CHANGE

  • The default theme.palette.info, theme.palette.success and theme.palette.warning is changed to have better contrast ratio.

    info = {
    - main: cyan[500],
    + main: lightBlue[700], // lightBlue[400] in "dark" mode
    - light: cyan[300],
    + light: lightBlue[500], // lightBlue[300] in "dark" mode
    - dark: cyan[700],
    + dark: lightBlue[900], // lightBlue[700] in "dark" mode
    }
    
    success = {
    - main: green[500],
    + main: green[800], // green[400] in "dark" mode
    - light: green[300],
    + light: green[500], // green[300] in "dark" mode
    - dark: green[700],
    + dark: green[900], // green[700] in "dark" mode
    }
    
    warning = {
    - main: orange[500],
    + main: "#ED6C02", // orange[400] in "dark" mode
    - light: orange[300],
    + light: orange[500], // orange[300] in "dark" mode
    - dark: orange[700],
    + dark: orange[900], // orange[700] in "dark" mode
    }

    To get the old behavior, use this colors in theme.

    import { cyan, green, orange } from '@material-ui/core/colors';
    
    const theme = createTheme({
      palette: {
        info: { main: cyan[500], light: cyan[300], dark: cyan[700] },
        success: { main: green[500], light: green[300], dark: green[700] },
        warning: { main: orange[500], light: orange[300], dark: orange[700] },
      }
    })

Preview: https://deploy-preview-26817--material-ui.netlify.app/customization/palette/#default-values

Issue

info success and warning default color does not pass AA contrast standard.

Current colors

Screen Shot 2564-06-18 at 16 42 45

Screen Shot 2564-06-18 at 16 45 22

Screen Shot 2564-06-18 at 16 46 06

Components that contains text are barely seen
Screen Shot 2564-06-18 at 16 47 32
Screen Shot 2564-06-18 at 16 50 06
Screen Shot 2564-06-18 at 16 47 00

Proposed Change

  • info: from cyan[500] to lightBlue[700]
  • success: from green[500] to green[800]
  • warning: from orange[500] to orange[800] (this is the best that orange can do 😭)

Screen Shot 2564-06-18 at 17 29 45

Screen Shot 2564-07-08 at 14 00 46

Screen Shot 2564-06-18 at 16 54 26

Better
Screen Shot 2564-06-18 at 16 59 50
Screen Shot 2564-06-18 at 17 01 30

https://codesandbox.io/s/alert-dark-light-colors-forked-e6m53

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

As we aren't yet reviewing our color palette, I'm comfortable with going forward with this by only assuring the contrast ratio. In the near future, we should be using our own tweaked palette!

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 27, 2021

we should be using our own tweaked palette

@danilo-leal Do you mean for the branding or the second theme? Agree!
For Material Design and the demos of it, I think that changing these colors before v6, 1+ year, could be a real challenge (breaking changes). So I think that we should make sure we get it right before v5 stable.

@siriwatknp Looking at the color, the main question I would have is on the <Alert>. The previous colors were optimized for this use case (the only component to use them in v4), which didn't work really well for the button/text field.
Now, the new colors work better with the button/text field, but worse with the alert.

Are the colors vivid enough? I personally don't feel to. It feels cold and grey.

Capture d’écran 2021-06-27 à 14 30 08

https://developers.google.com/maps/documentation/javascript/overview

vs.

Capture d’écran 2021-06-27 à 14 30 28

Capture d’écran 2021-06-27 à 14 42 30

I think that we should re-energize them. How? I don't know. Maybe it's the point where 3 colors are not enough (light, main, dark), and we need a larger scale: 100 to 900. Maybe this is enough?

diff --git a/packages/material-ui/src/Alert/Alert.js b/packages/material-ui/src/Alert/Alert.js
index f13e17cd53..3699068213 100644
--- a/packages/material-ui/src/Alert/Alert.js
+++ b/packages/material-ui/src/Alert/Alert.js
@@ -55,8 +55,8 @@ const AlertRoot = styled(Paper, {
     /* Styles applied to the root element if variant="standard". */
     ...(color &&
       styleProps.variant === 'standard' && {
-        color: getColor(theme.palette[color].main, 0.6),
-        backgroundColor: getBackgroundColor(theme.palette[color].main, 0.9),
+        color: getColor(theme.palette[color].light, 0.6),
+        backgroundColor: getBackgroundColor(theme.palette[color].light, 0.86),
         [`& .${alertClasses.icon}`]: {
           color: theme.palette[color].main,
         },

Capture d’écran 2021-06-27 à 14 47 13


Windows 11 https://twitter.com/stroughtonsmith/status/1408984689092739072

20210628_014835

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 28, 2021

Are the colors vivid enough? I personally don't feel to. It feels cold and grey.

then, I propose this change for Alert variants. use color[500] as light in the palette, so that the Alert mostly look the same.

  • standard => use light
  • outlined => use light
  • filled => use main (I think light to not good with white text color, but no strong opinion if we want to keep the same looks and feel)

here is the result.

variant: standard (same as v4)

Screen Shot 2564-06-28 at 10 14 36

Screen Shot 2564-06-28 at 10 19 13

variant: outlined (same as v4)

Screen Shot 2564-06-28 at 10 14 47

Screen Shot 2564-06-28 at 10 19 32

variant: filled (darker from v4)

Screen Shot 2564-06-28 at 10 15 00

Screen Shot 2564-06-28 at 10 38 29

@siriwatknp siriwatknp requested a review from mnajdova June 28, 2021 13:20
Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

@siriwatknp
I have made some tweaks to this, check the Code Sandbox here.

It seems weird using Cyan as an info color, I'd figure that gray is a more unopinionated color for information. Also proposing yellow as a warning color, which seems more suitable than orange. Made sure to pass at least from 3:1 contrast ratio.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 30, 2021

gray is a more unopinionated color

@oliviertassinari what do you think? I feel grey is too much breaking change (even though this PR is already a breaking change). It might be better to try grey on the 2nd design system than in material design v5.

Also proposing yellow as a warning color

yellow is pretty bad. https://codesandbox.io/s/alert-dark-light-colors-forked-kgocn?file=/demo.js

It looks like a different color between light & dark.

@danilo-leal
Copy link
Contributor

It looks like a different color between light & dark.

I've just corrected that one, it was indeed with different shades between light and dark. I feel like we could go on with the same!

@oliviertassinari what do you think? I feel grey is too much breaking change (even though this PR is already a breaking change). It might be better to try grey on the 2nd design system than in material design v5.

Why would you say this color change is a breaking one? Material Design as long as I know doesn't actually define info, warning, and success color, doesn't it? They generate them from the Primary and Secondary palettes. What breaks if we change it from cyan to gray?

@siriwatknp
Copy link
Member Author

What breaks if we change it from cyan to gray?

For new people starting with v5 it is totally fine. The breaking change is for people on v4 migrating to v5, from my experience, material-ui appears a lot in dashboard app which frequently needs info color and I assume that some of the project use default cyan color.

Anyway, I have another idea. I am fine with your proposal on grey if @oliviertassinari has no strong opinion. For people on v4, I will add v4 default color to adaptV4Theme so that developer can incrementally fix the breaking changes. Is that sound good?

I assume that people will use codemod (which I already added adaptV4Theme as one of codemod).

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 1, 2021

Regarding the info color. I think that using grey vs. cyan depends on the design direction we want to take. If we want to stay closer to Google's realm, I think that blue over the two would match better, e.g.

Capture_d’écran_2021-06-02_à_14 32 31

Regarding the warning color. I also think that yellow is more common than orange.

I have tried a POC on https://codesandbox.io/s/alert-dark-light-colors-forked-l488p?file=/demo.js tweaking some of the values.

@siriwatknp
Copy link
Member Author

I have tried a POC on https://codesandbox.io/s/alert-dark-light-colors-forked-l488p?file=/demo.js tweaking some of the values.

I am fine with this, it looks like orange anyway 😂

@siriwatknp
Copy link
Member Author

siriwatknp commented Jul 7, 2021

@oliviertassinari @danilo-leal Do we have the conclusion about info color? should we go with cyan for now (info from Olivier about Google is the only thing that convince me).

for Danilo's suggestion, we can put it in 2nd design system.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 7, 2021

@siriwatknp IMHO, we should use blue over cyan and grey to stay closer to Google's products.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jul 8, 2021

@siriwatknp IMHO, we should use blue over cyan and grey to stay closer to Google's products.

will go with lightBlue instead (blue is the same color as primary), is that sounds good to you?

https://codesandbox.io/s/alert-dark-light-colors-forked-e6m53

also I added v4 color in adaptV4Theme to preserve the same color when migrating to v5.
I don't think this is needed, otherwise it will cause confusion why adaptV4Theme only apply old behavior for state colors but not primary & secondary. I think the colors is better in both light & dark theme, so the projects that use default theme are definitely benefit from it. (Even in worse case they don't like it, they can always use the colors in migration docs to get the old behavior)

@siriwatknp siriwatknp merged commit f5d0366 into mui:next Jul 9, 2021
@siriwatknp siriwatknp deleted the fix/default-state-color branch July 9, 2021 13:57
@danilo-leal
Copy link
Contributor

@siriwatknp IMHO, we should use blue over cyan and grey to stay closer to Google's products.

Looks good to me :)

@siriwatknp
Copy link
Member Author

@siriwatknp IMHO, we should use blue over cyan and grey to stay closer to Google's products.

Looks good to me :)

I merged with lightBlue instead because blue is already used as primary

@oliviertassinari
Copy link
Member

@siriwatknp Sounds fair 👍

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 10, 2021

The changes we did in the Alert.tsx feels like a hack but it's probably the best/simplest we can hope for now. I can't wait for figuring out a better story around the usage of the design token of the palette, maybe in v6 or even before with the second design system.

@oliviertassinari oliviertassinari changed the title [Theme] update default info success warning color [Theme] Update default info success warning color Jul 11, 2021
@oliviertassinari oliviertassinari mentioned this pull request Jul 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants