[core] Revise the theme.palette.primary & secondary approach #9794

mbrookes · 2018-01-08T22:03:45Z

Closes #8075
Closes #8183

Still outstanding:

~~Two uses of primary accent colors (currently converted to primary.dark / light)~~
(ok as is)
~~Enhanced dark / light method?~~
(Not needed)
Revert the docs secondary color change, it's just for testing with.
(Although personally I like orange better with our current blue primary!)
FIx flow errors & reinstate flow.

Breaking change

It's an important simplification of the palette system. You can now directly use the “official” Color Tool.

Instead of using a rich color object of 14 different keys, we rely on an object of 4 different keys: light, main, dark and contrastText.
Providing the full-color object used to be required. Now, we will provide a nice default to the different values using the main value.

import { createMuiTheme } from 'material-ui/styles';
import blue from 'material-ui/colors/blue';
import pink from 'material-ui/colors/pink';

const theme = createMuiTheme({
  palette: {
-   primary: blue,
-   secondary: pink,
+   primary: {
+     light: blue[300],
+     main: blue[500],
+     dark: blue[700],
+   },
+   secondary: {
+     light: pink[300],
+     main: pink[500],
+     dark: pink[700],
+   }
    type: theme.paletteType,
  },
});

oliviertassinari

Awesome start :)

oliviertassinari · 2018-01-09T07:02:54Z

docs/src/modules/styles/getPageContext.js

@@ -4,14 +4,14 @@ import { create, SheetsRegistry } from 'jss';
 import rtl from 'jss-rtl';
 import { createMuiTheme, createGenerateClassName, jssPreset } from 'material-ui/styles';
 import blue from 'material-ui/colors/blue';
-import pink from 'material-ui/colors/pink';
+import orange from 'material-ui/colors/orange';


Wait no, please revert, the contrast was 4.01 and it's 2.16 now.

Dude, please read: #9794 (comment) It's temporary. 😄 (But if we were going to change this, darkOrange might be a nice compromise.)

oliviertassinari · 2018-01-09T07:10:43Z

package.json

    "test:unit": "cross-env NODE_ENV=test mocha test/**/*.spec.js src/{,**/}*.spec.js",
    "test:watch": "yarn test:unit -w",
    "test:coverage": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha test/**/*.spec.js src/{,**/}*.spec.js && nyc report -r lcovonly",
    "test:coverage:html": "cross-env NODE_ENV=test BABEL_ENV=coverage nyc mocha test/**/*.spec.js src/{,**/}*.spec.js && nyc report --reporter=html",
    "test:karma": "cross-env NODE_ENV=test karma start test/karma.conf.js --single-run",
    "test:regressions": "webpack --config test/regressions/webpack.config.js && rimraf test/regressions/screenshots/chrome/* && vrtest run --config test/vrtest.config.js --record",
    "typescript": "tsc -p tsconfig.json",
-    "flow": "flow --show-all-errors",
+    "flow": "echo \"No flow.\"",


Wait. We still use internally for some of our files. Please put it back.

Relax ☺️, it's just to get through CI to check everything else. Flow was giving me the runaround over something trivial. I ran out of patience with it at 1:00am.

oliviertassinari · 2018-01-09T07:15:00Z

src/Form/FormLabel.js

@@ -4,7 +4,8 @@ import classNames from 'classnames';
 import withStyles from '../styles/withStyles';

 export const styles = theme => {
-  const focusColor = theme.palette.primary[theme.palette.type === 'light' ? 'A700' : 'A200'];
+  // FIXME: 'dark' & 'light' were accent colors before the theme change


https://material.io/guidelines/components/text-fields.html#text-fields-states

I think that we are good, you can remove the comment:

oliviertassinari · 2018-01-09T07:20:45Z

src/Input/Input.js

@@ -74,7 +74,8 @@ export const styles = theme => {
    },
    inkbar: {
      '&:after': {
-        backgroundColor: theme.palette.primary[theme.palette.type === 'light' ? 'A700' : 'A200'],
+        // FIXME: 'dark' & 'light' were accent colors before the theme change


It's reducing entropy 👍 .

oliviertassinari · 2018-01-09T07:24:32Z

src/Progress/LinearProgress.js

@@ -13,27 +13,27 @@ export const styles = theme => ({
    height: 5,
  },
  primaryColor: {
-    backgroundColor: theme.palette.primary[100],
+    backgroundColor: theme.palette.primary.light,


The color is too close to theme.palette.primary.main. We can't see the difference:

I think that we need a lighten() call.

@oliviertassinari:
We can use A200 for A100

Yeah, I had my doubts about that. lighten() it is.

oliviertassinari · 2018-01-09T07:27:22Z

src/styles/createPalette.js

-    secondary = pink,
-    error = red,
+    primary = {
+      light: indigo[300],


Question, why using 300 and not a lighter version of indigo[500]?
(we could just remove this line so the default logic quicks in)

Bacuase it's the "correct" color according to the MD spec. But we can approximate if you're happy with that.

Sounds good :)

Sorry, which?

@mbrookes Your argument sounds good. I thought MCW was doing the opposite.

oliviertassinari · 2018-01-09T07:28:04Z

src/styles/createPalette.js

    type = 'light',
    // Same value used by material-components-web
    // https://github.com/material-components/material-components-web/blob/ac46b8863c4dab9fc22c4c662dc6bd1b65dd652f/packages/mdc-theme/_functions.scss#L49
-    contrastThreshold = 3.1,
+    contrastThreshold = 3,


3.1 so we better follow the specification. Compare https://material-ui.com/style/color/#color with https://material.io/guidelines/style/color.html#color-color-palette.

"It's déjà vu all over again". 😄

#8183 (comment)

That isn't the spec for contrast color it's the spec for the color pallette. The text is simply labels, so it doesn't matter that they're different.

our current color tables' labels don't match the spec in places if you compare them.

This is the best default for end users IMHO. If you really want the docs color table labels closer to the labels in the spec (contrastThreshold = 3.1 doesn't exactly match either), we can override it in the docs.

I won't argue for 0.1. Fine.

mbrookes · 2018-01-12T21:23:15Z

@oliviertassinari I think we're nearly there. The only thing I'm wondering is if we should add error.accent: red.A400, and make error.main: red[500].

If so, we could do the same for primary / secondary, and keep the Input underline etc. consistent with the current implementation.

oliviertassinari · 2018-01-12T21:50:35Z

@mbrookes Why do we use two different error colors in the first place 😮 ? Something is wrong here, nothing is justifying us using two different values here 🙈 :

I could only find one occurrence of the error color on material-components-web side, they are using red.A700.
The specification is encouraging using A400. Bootstrap has only one alert color.

mbrookes · 2018-01-12T22:00:03Z

I don't know why for sure, but it does seem red.A400 is a bit "harsh" for <Icon color="error" />, while red[500] is perhaps a bit dull for error text. If you're happy to standardize, lets pick one and go with it.

#8075 (comment) Closes 8075 Revise the theme.palette.primary & secondary approach Update & expand createPalette tests Update createMuiTheme tests Bump package size

oliviertassinari · 2018-01-13T13:55:35Z

@mbrookes Yeah 🎉 ! Good job 👯‍♂️

yuchi · 2018-01-13T14:30:38Z

I love you guys ❤️

oliviertassinari · 2018-01-13T14:34:31Z

@yuchi Half the value comes from you raising the issue and convincing us to do the change 💯 .

oliviertassinari added core breaking change labels Jan 9, 2018

oliviertassinari reviewed Jan 9, 2018

View reviewed changes

oliviertassinari assigned mbrookes Jan 11, 2018

mui deleted a comment from oliviertassinari Jan 12, 2018

mbrookes added 10 commits January 12, 2018 23:39

[core] Revise the theme.palette.primary & secondary approach

30e4ea4

#8075 (comment) Closes 8075 Revise the theme.palette.primary & secondary approach Update & expand createPalette tests Update createMuiTheme tests Bump package size

[core] colorManipulator refactoring

b3a0b34

[FormLabel][Input] Remove FIXMEs

13955a7

[core] Better tonalOffset values

5506496

Fix 100 shades

2a2a242

[docs] Fix demos

ee33b54

[tests] Reinstate flow, fix flow errors

64ba64f

[docs] Apply standard colors to primary light & dark

35e3166

[Icon][SvgIcon] Fix error color

8f16e10

[tests] Bump size limit. Again!

f5a1e75

oliviertassinari mentioned this pull request Jan 13, 2018

Primary color does not match the theme #9857

Closed

1 task

oliviertassinari approved these changes Jan 13, 2018

View reviewed changes

oliviertassinari added the PR: accepted label Jan 13, 2018

mbrookes merged commit eb36957 into mui:v1-beta Jan 13, 2018

mbrookes mentioned this pull request Jan 13, 2018

Custom primary color. #9860

Closed

xaviergonz mentioned this pull request Jan 14, 2018

[Typescript] Palette and PaletteOptions are not in sync with changes to palette #9874

Closed

1 task

yzorg mentioned this pull request May 1, 2018

Using primary/accent colors at different levels #9064

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Revise the theme.palette.primary & secondary approach #9794

[core] Revise the theme.palette.primary & secondary approach #9794

mbrookes commented Jan 8, 2018 •

edited by oliviertassinari

Loading

oliviertassinari left a comment

oliviertassinari Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018

mbrookes Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018

mbrookes Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018

oliviertassinari Jan 9, 2018

oliviertassinari Jan 9, 2018

oliviertassinari Jan 9, 2018

mbrookes Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018

mbrookes Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited

Loading

mbrookes Jan 9, 2018

oliviertassinari Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited by zannager

Loading

mbrookes Jan 12, 2018 •

edited

Loading

oliviertassinari Jan 12, 2018 •

edited

Loading

mbrookes commented Jan 12, 2018

oliviertassinari commented Jan 12, 2018 •

edited

Loading

mbrookes commented Jan 12, 2018

oliviertassinari commented Jan 13, 2018 •

edited

Loading

yuchi commented Jan 13, 2018

oliviertassinari commented Jan 13, 2018 •

edited

Loading

[core] Revise the theme.palette.primary & secondary approach #9794

[core] Revise the theme.palette.primary & secondary approach #9794

Conversation

mbrookes commented Jan 8, 2018 • edited by oliviertassinari Loading

Breaking change

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2018 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 9, 2018 • edited by zannager Loading

Choose a reason for hiding this comment

mbrookes Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

mbrookes commented Jan 12, 2018

oliviertassinari commented Jan 12, 2018 • edited Loading

mbrookes commented Jan 12, 2018

oliviertassinari commented Jan 13, 2018 • edited Loading

yuchi commented Jan 13, 2018

oliviertassinari commented Jan 13, 2018 • edited Loading

mbrookes commented Jan 8, 2018 •

edited by oliviertassinari

Loading

oliviertassinari Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited

Loading

mbrookes Jan 9, 2018 •

edited

Loading

mbrookes Jan 9, 2018 •

edited

Loading

mbrookes Jan 9, 2018 •

edited

Loading

mbrookes Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited

Loading

oliviertassinari Jan 9, 2018 •

edited by zannager

Loading

mbrookes Jan 12, 2018 •

edited

Loading

oliviertassinari Jan 12, 2018 •

edited

Loading

oliviertassinari commented Jan 12, 2018 •

edited

Loading

oliviertassinari commented Jan 13, 2018 •

edited

Loading

oliviertassinari commented Jan 13, 2018 •

edited

Loading