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

[style] Add missing blueGrey colors #6548

Merged
merged 1 commit into from
Apr 8, 2017

Conversation

peteratticusberg
Copy link
Contributor

the blueGrey color is missing the colors A100, A200, A400, and A700 as well as the contrastDefaultColor setting which causes Material UI to throw an error if it was chosen as the primary or accent color.

This adds those in :)

A200: '#b0bec5',
A400: '#78909c',
A700: '#455a64',
contrastDefaultColor: 'light',
Copy link
Member

@oliviertassinari oliviertassinari Apr 8, 2017

Choose a reason for hiding this comment

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

Where have you found those colors? I can't find them in the spec https://material.io/guidelines/style/color.html#color-color-palette. Also, we would need to update the docs if we move forward.

@peteratticusberg
Copy link
Contributor Author

Ah, good points on both counts.

Brown is also missing colors A100, A200, etc. in the spec and the way we dealt with that was by just setting A100 to the same color as 100, A200 to the same color as 200, etc. I saw that and just did the same thing for blue grey ¯_(ツ)_/¯.

export const brown = {
  50: '#efebe9',
  100: '#d7ccc8',
  200: '#bcaaa4',
  300: '#a1887f',
  400: '#8d6e63',
  500: '#795548',
  600: '#6d4c41',
  700: '#5d4037',
  800: '#4e342e',
  900: '#3e2723',
  A100: '#d7ccc8',
  A200: '#bcaaa4',
  A400: '#8d6e63',
  A700: '#5d4037',
  contrastDefaultColor: 'brown',
};

I propose we do one of the following:

  1. Remove Blue Grey, Brown and Grey from colors.js. None of them have defined A* values in the material-ui spec, we don't want to invent A* colors, and colors without A* values break themes.

  2. Invent our own temporary A* values for Brown, Blue Grey, and Grey for the time being that approximate what those values might be. After looking at the spec, the mapping I did here is not accurate. An improved version would look something like A100 -> 200, A200 -> 400, A400 -> 600, and A700 -> 800.

What do we think?

I feel like (1) is probably the right thing to do though I'm using Blue Grey for a project of mine so I'm tempted to do (2).

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 gonna approve that PR for consistency with the other colors.
I would rather keep those colors and make apply an interpolation function util we the specification is explicit. What do you think?

@oliviertassinari oliviertassinari merged commit 9fe25c2 into mui:next Apr 8, 2017
@peteratticusberg
Copy link
Contributor Author

I think this is fine for now :)

If we were going to go as far to make a special function for computing the A* variants of a color, I'd think it'd also be worth creating methods to generate the shades 50-700 based off a single color as that would enable users to specify a single color for which we could auto-generate a whole palette.

@oliviertassinari oliviertassinari changed the title adds accent colors + a default contrast color to blueGrey [style] Add accent colors + a default contrast color to blueGrey Apr 14, 2017
@oliviertassinari oliviertassinari changed the title [style] Add accent colors + a default contrast color to blueGrey [style] Add missing blueGrey colors Apr 14, 2017
@oliviertassinari oliviertassinari added the design: material This is about Material Design, please involve a visual or UX designer in the process label Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants