Material-ui: RFC: color prop API

[eps1lon]

Currently our type declarations contain the following definition for color props:

type Color = 'inherit' | 'primary' | 'secondary' | 'default';

indicating that there is a library wide understanding what these color represent and that every component that has a color prop should implement each variant.

However only primary and secondary are implemented for every component with a color prop. inherit and default are not implemented in every component. default doesn't even have a consistent style.

Implementation overview

| Component | primary | secondary | inherit | default |
|------------|---------|-----------|---------|---------|
| AppBar | x | x | x | x |
| Badge | x | x | | x |
| Button | x | x | x | x |
| Chip | x | x | | x |
| Icon | x | x | x | |
| IconButton | x | x | x | x |
| SvgIcon | x | x | x | |
| Typography | x | x | x | x |

default variant

Implementation

| Component | color | background-color |
|------------|---------------------------------------------------------|-----------------------------------------------------------------------------------------|
| AppBar | theme.palette.getContrastText(backgroundColorDefault) | theme.palette.type === 'light' ? theme.palette.grey[100 ] : theme.palette.grey[900 ] |
| Badge | theme.palette.textColor (which is undefined) | theme.palette.color (also undefined) |
| Button | theme.typography.button | global stylesheet |
| Chip | theme.palette.getContrastText(backgroundColor) | theme.palette.type === 'light' ? theme.palette.grey[300 ] : theme.palette.grey[700 ] |
| IconButton | theme.palette.action.active | fade(theme.palette.action.active, theme.palette.action.hoverOpacity) if :hover |
| Typography | global stylesheet | global stylesheet |

Proposal

Remove it because:

• not even the actual default value for the components
• not mentioned in the material spec
• broken for Badge with no report (I was not able to determine when this actually broke but I guess this happened a few months ago; Edit: passed undefined even in 1.0.0-alpha.2)

People can always set the color prop to undefined which will result in no applied css rules concerning color which is a proper default in my opinion.

inherit variant

Implementation

| Component | color | backgroundColor |
|------------|-------------------|-------------------|
| AppBar | global stylesheet | global stylesheet |
| Button | inherit | global stylesheet |
| Icon | global stylesheet | global stylesheet |
| IconButton | inherit | global stylesheet |
| SvgIcon | global stylesheet | global stylesheet |
| Typography | inherit | global stylesheet |

Funny enough in Icon fontSize="inherit" color="inherit" causes font-size: inherit; but no defined color in css.

Also the default for fontSize in those components is default and applies __always__ no css rules but the default for color is inherit which applies __sometimes__ no css rules. This might as well be removed. There is no value in a named default value in my opinion but this is should be discussed separately.

Proposal

No strong opinion about that one. Either repurpose this as a default replacement which means color and background-color are not set or actually set inherit which is the most obvious. AppBar for example does not do anything with inherit which might be confusing.

/cc @mui-org/core-contributors

[mbrookes]

default doesn't even have a consistent style

The default color should be whatever is consistent with the spec if no alternative color enum is supplied, but as you suggest, there doesn't need to be a default variant for that.

Removing it would be a breaking change though, so deprecation warnings required.

[eps1lon]

Removing it would be a breaking change though, so deprecation warnings required.

This will only affect the AppBar. For the other components "default" would just be an alias for undefined.

Badges are currently in a weird sport because "default" is broken anyway (or maybe not since I don't know how they looked). Maybe that's a good thing because nobody would mourn about a breaking change.

The versioning would look like this:

• minor: deprecate "default" for AppBar; make it a no-op for the other components (it basically is that already).
• major: remove "default" (We could leave it as a no-op but I'm not a fan of that. Just increases debt with time.)

[oliviertassinari]

default variant

The default variant is the value that matches the specification. It's the color value you would expect by default. It's not consistent by design. What do we win by removing it? How do you solve the problem instead?

Regarding the Badge, agree, it seems wrong, I don't think that a default variant makes sense. I think that we could rename default -> inherit.

People can always set the color prop to undefined which will result in no applied css rules concerning color which is a proper default in my opinion.

Is it how you are suggesting to solve the problem? Would 'default' be more explicit than undefined? Let say I set color={undefined} Wouldn't you expect the color to inherit?

inherit variant

Funny enough in Icon fontSize="inherit" color="inherit" causes font-size: inherit; but no defined color in css.

Yes, it's expected, we rely on the default browser value. Isn't that correct?

---

To sum up, I think that we can fix the Badge implementation and we should be good.

[oliviertassinari]

Oh, the AppBar is an interesting case. The specification changed, having a default value here doesn't make sense anymore (it's not even the default value). I think that we can remove it for this component.

[eps1lon]

Yes, it's expected, we rely on the default browser value. Isn't that correct?

@oliviertassinari
It's inconsistent. Sometimes it sets the css property to inherit sometimes it doesn't set anything. Those are two different things. I would expect that setting "inherit" in a prop to mean the same thing across components.

So it's how you are suggesting to solve the problem instead. Isn't 'default' more explicit than undefined? Let say I set color={undefined} Wouldn't you expect the color to inherit? Well, it won't.
By using 'inherit' and 'default', we are making the behavior deterministic.

It doesn't matter what I expect. The runtime already falls back to whatever is defined as a default value if you pass undefined as an argument to a function. If I'm used to that behavior when using javascript I'm used to that behavior in whatever is written in javascript.

TL;DR: Remove "default" from AppBar, set the default of Badge to inherit followed by some rant about stringly typing a default value.

I have never seen a component API that used a string literal to instruct that the component should use its default behavior. My main issue is with facebook's understanding of what an optional type is. Using PropTypes.oneOf(["default", ...colors]) will allow "default" | undefined | null | typeof colors. An explicit undefined | "default" is equivalent. Setting null causes a runtime error without a prop-types warning.

What I don't understand is why the current API is easier to understand than ProTypes.oneOf(colors) and setting the color className if color != null. Setting an argument to undefined is saying to the runtime that it should fallback to the default value. That is true for javascript as a language so I think we should expect from users to know that.

Or would you design the signature of a function so that users can write

function multiply(a, b = "default") {
  const c = b === "default" ? 1 : b;
  return a * c;
}

I think this is really confusing if we set the default value of most components to the literal "default" and on AppBar to "primary". So for some components the default is "default" and for some it's "primary" and then for Badge it's "inherit". Naming a default value if there is no correspondence to an actual named color variant like primary, secondary, action etc. just seems arbitrary.

It's also bad because "primary" means the same for the color across components. That is not true for "default". Depending on the component it's either some shade of grey or black with some transparency.

Edit:
Also the default variant in TextField is not called "default" but "standard".

[oliviertassinari]

I would expect that setting "inherit" in a prop to mean the same thing across components.

It does mean the same thing: As a user, you can set a color on the parent and see it being applied to the children. Same output, no matter the implementation. What changed, is that in some cases, we have to add some extra CSS, in some other cases we don't need it. It's an implementation detail.

It doesn't matter what I expect.

It's about having the default behavior documented in the AP. Sometimes, the color inherit: inherit, sometimes it's a specific value defined by the specification: default. I find it wrong to set a color value in the CSS while having color = undefined, if we set a value, it should have a meaningful label, maybe default should be renamed spec?

[eps1lon]

It's an implementation detail.

Fair enough.

I find it wrong to set a color value in the CSS while having color = undefined.

That is the current behavior though. undefined and "default" are equivalent.

It's about having the default behavior documented in the API.

That is not the case currently. I don't know what color is applied when I use "default". I know what "primary", "secondary" and "error" means if I know the material design palette. I know what action means which is an addition in this implementation. "default" however is not defined. "background" and "surface" might make sense for some components but those are currently not implemented.

For example https://material-ui.com/api/icon-button/ has separate classes for each color category except "default". It does not say which class is applied for the "default" category. So I guess putting them in root is fine. But then those rules are also applied if I use a different category. This is a special behavior of the string literal "default".

"The color of the component. It supports those theme colors that make sense for this component."

"default" is not a theme color so where is this taken from? The same argument could be made for "inherit" but I'm ok with the tradeoff here since it's related to css inherit.

[oliviertassinari]

That is not the case currently. I don't know what color is applied when I use "default"

@eps1lon In this case, what about using an explicit value by renaming the default value? I really like this approach too.

[eps1lon]

Let's take https://material.io/design/color/the-color-system.html#color-theme-creation as the baseline. We have primary, secondary, surface and background and error and call them categories.

primary and secondary have light, main and dark variants as well as emphasis variants that are used for buttons. high defaults to dark, normal to main and low would be what is currently used for "default" (see https://material.io/design/components/buttons.html#hierarchy-placement)

Each category + variant combination has a contrast color which is named as On + category.

I believe that the API is simply <Component color="primary" variant="dark" /> and the implementation just™ has to pick from the palette. Everything that is currently used from the grey palette is just™ On* with opacity. For buttons one would use emphasis instead of variant.

I would hope that there is no need for helpers like getContrastText or fade other than in createPalette.

This is at least how I interpret the current spec.

[oliviertassinari]

@eps1lon I gave this issue a second read. I have changed my mind. I do no longer have a strong opinion in any direction. I like the idea of replacing the default value with the actual value used.

• AppBar: default -> grey
• Badge: default -> inherit
• Button: default -> textPrimary
• IconButton: default -> actionActive
• Typography: default -> textPrimary
• Switch: default -> grey

Now, with the introduction of the system package. I think that we should work in the direction of integrating the package with the core components. Meaning having all the scope of the colors available, like <Paper color="text.secondary" />. In this logic, I think that it would be more consitant to rename textPrimary -> text.primary, actionActive -> action.active, textSecondary -> text.secondary, etc.

[eps1lon]

@oliviertassinari This is something I'd like to revisit once @material-ui/styles is stable. Styles as a function of props will save as quite a bit of logic and a consistent color API simply emerges from styles as a function of props. I'm not a big fan of string paths since typings support is not as trivial as simple string literals. I'd need to take a closer look to see how this works with builtin themes and custom themes.

I'd like to have a look at the color palette first since the concept is not very close to the material color system anyway. There is no such thing as text.primary but rather "on"-colors. So you would use onPrimary rather than text.primary. There is a distinction because "on"-colors are also used for icons and other elements and not just text. Using text.primary for an icon wouldn't make sense from a semantics standpoint.

I'd really like for designers that are familiar with material design to "just" pick up our theming and build new ones without having to study our API docs to see what color corresponds to what in the components. If we state that we implement material design then we should make every effort to make the transition as smooth as possible and wherever we deviate we should have a strong argument for it and at least an adapter for it.

[smmoosavi]

The light color type is needed for the Typography component.

[skirunman]

Please don't remove the inherit color property as we make extensive use for custom theming of our app. Thanks!

[oliviertassinari]

@skirunman inherit is a must-have, there is no plan in losing this capability. I think that the concern is about how we harmonize the implementation.