[design-system] Support error|success|warning|info by default in color prop

[michael-land]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

It already supported out of box in V5.

https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Button/Button.js#L137
https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Button/Button.js#L116
https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/Button/Button.js#L116

User could add it to theme via interface merge, but it would be nice to add it in core

declare module '@material-ui/core/Button' {
  interface ButtonPropsColorOverrides {
    error: true;
    info: true;
    success: true;
    warning: true;
  }
}

[eps1lon]

Are you asking for this behavior in v4 or v5?

I was under the impression we already support in @material-ui/core@5.0.0-alpha.24. If it's not working , can you share a reproducible example?

We will not backport this feature to v4.

[michael-land]

I'm referring v5, https://codesandbox.io/s/material-ui-issue-forked-ce2vj?file=/src/Demo.tsx

The expected behavior is no type error.

[povilass]

Augmentation works, you are doing it wrong codesandbox.

You need to import interface with augmentated type, same goes for Badge.

import { ButtonPropsColorOverrides } from "@material-ui/core/Button/Button";

declare module "@material-ui/core/Button/Button" {
  interface ButtonPropsColorOverrides {
    error: true;
    info: true;
    success: true;
    warning: true;
  }
}

[michael-land]

it should not raise error without augmentated type.

Mui already support these variations in V5, why user need to add these colors to theme type manually?

[povilass]

The component uses only some colors from the palette which are type of PaletteColor only. So maybe it makes sense that those colors should be included in this array. Look at playground, I created type called PaletteColorKeys which only picks keys like primary, secondary, myNewColor and so on which extands PaletteColor but not "action" or "background". In this way, you don't new to implement augmentation even. @eps1lon

Also for generating ButtonProps classes typescript lets us do stuff like this with string literal types.
Playground

[oliviertassinari]

Interesting, thanks for the discussion. It makes me think about the discussion that was started by Sebastian in #13028.

How about we aim for?

diff --git a/packages/material-ui/src/Button/Button.d.ts b/packages/material-ui/src/Button/Button.d.ts
index 3b5666d1d6..692f40bc5e 100644
--- a/packages/material-ui/src/Button/Button.d.ts
+++ b/packages/material-ui/src/Button/Button.d.ts
@@ -1,6 +1,6 @@
 import { OverridableStringUnion } from '@material-ui/types';
 import { SxProps } from '@material-ui/system';
-import { Theme } from '../styles';
+import { PropTypes, Theme } from '..';
 import { ExtendButtonBase, ExtendButtonBaseTypeMap } from '../ButtonBase';
 import { OverrideProps, OverridableComponent, OverridableTypeMap } from '../OverridableComponent';

@@ -100,10 +100,7 @@ export type ButtonTypeMap<
      * The color of the component. It supports those theme colors that make sense for this component.
      * @default 'primary'
      */
-    color?: OverridableStringUnion<
-      Record<'inherit' | 'primary' | 'secondary', true>,
-      ButtonPropsColorOverrides
-    >;
+    color?: OverridableStringUnion<Record<PropTypes.Color, true>, ButtonPropsColorOverrides>;
     /**
      * If `true`, the component is disabled.
      * @default false
diff --git a/packages/material-ui/src/index.d.ts b/packages/material-ui/src/index.d.ts
index 3f3f0ad5e8..8b34e775ba 100644
--- a/packages/material-ui/src/index.d.ts
+++ b/packages/material-ui/src/index.d.ts
@@ -68,7 +68,7 @@ export interface Color {
 export namespace PropTypes {
   // keeping the type structure for backwards compat
   // eslint-disable-next-line @typescript-eslint/no-shadow, @typescript-eslint/no-unused-vars
-  type Color = 'inherit' | 'primary' | 'secondary' | 'default';
+  type Color = 'inherit' | 'primary' | 'secondary' | 'success' | 'error' | 'info' | 'warning';
 }

 // From index.js

[povilass]

@oliviertassinari yes it could work but I can pass my own created colors from in palette keys using PaletteColor type, like "tertiary", "silver" and more, Button component actually even now can do it no changes needed, the problem only with typings how to provide it.
The biggest problem I see is class generation:

1. Create a type

import { Palette, PaletteColor } from "@material-ui/core/styles";

type PaletteColorKeys = keyof Pick<
  Palette,
  {
    [P in keyof Palette]: Palette[P] extends PaletteColor ? P : never;
  }[keyof Palette]
>;

2. DefaultPaletteColorKeys = 'primary' | 'secondary' | 'success' | 'error' | 'info' | 'warning'; // for class generation and slots
   Generate classes only by default keys if someone implements there own do not generate them.

3. Apply to button props, color property as PaletteColorKeys, and classes for default palette classes.

Playground

No breaking changes and you introduced a really good API.

And ButtonPropsColorOverrides not needed anymore for augmentation (only if you want to disable colors).

[oliviertassinari]

@povilass Are you saying that extending the theme palette in TypeScript for new colors should automatically extend the components to be able to use them?

It won't always make sense. There are keys in the theme palette that aren't or won't even be compatible or that designers don't want to blindly all allow, e.g. <AppBar color="error">.

I wonder. We don't have to solve this problem now, we could defer it to later, it will allow us to make progress with the breaking changes first, the release of v5 stable as well as leaving more time for the idea to grow.

Fundamentally, I think that we can frame this as a tradeoff. Should we prioritize flexibility (creativity) or soundness (pit of success)?

Edit: with hindsight working on the branding pages, the former seems more valuable. It empowers creativity.
As for the soundness, we still allow developers to flag values as invalid, so if they care, they can or create a wrapper that limits the options. I think that a team needs you to be of a nonnegligible size to start carrying about this aspect (>5 people working on the DS?)

[povilass]

@oliviertassinari But you won't change anything because everyone adds ButtonPropsColorOverrides (or other ColorOverides) with augmentation it's just simpler to use it out of the box without boilerplate. In most cases, you would like to use all colors in all components without extra work and it will reduce extra support for those kinds of things.

Of course, the downside is are you actually want to allow/disallow usage color in that component. It is reverse logic what we have now with augmentation everything is disabled except default colors after change it will be enabled by default. Question is it ok to let or not.

[oliviertassinari]

@povilass How about we move forward with #24778 (comment) but delay the idea to automatically support in TypeScript any theme.palette properties added in the theme?

Supporting the existing colors in the theme is aligned with the idea to have added success/error/info/warning in the palette in the first place.

At this point, my main concern is that we can't easily know which keys are compatible. In our rebranding many keys aren't: https://github.com/mui-org/material-ui/blob/b8c6dee91abd4919bb87ae87a1929dd8f8fcb830/docs/src/modules/branding/BrandingRoot.tsx#L54

I also believe that we should encourage design consistency, designer shouldn't need many different new colors.

[povilass]

@oliviertassinari yes of course we can delay till the next iteration in the version or something.

[mngu]

I really like @povilass solution to this! Will it make the solution in #13875 obsolete ?

At this point, my main concern is that we can't easily know which keys are compatible

@oliviertassinari Keys that extends PaletteColor should do ?
Willing to make a PR in this direction, since it is a well requested feature.

[oliviertassinari]

I really like @povilass solution to this! Will it make the solution in #13875 obsolete ?

@mngu I don't think it makes the solution obsolete. Even if we inverse the logic, there is always a need to restrict the customization options available.

[povilass]

@mngu I excepted as much that if you implement new colour it would be easier to use it, but if material UI focus on restricting theme extension it's fine I think.

[mngu]

A quick example on Button => next...mngu:support-custom-palette

[oliviertassinari]

@mngu Thanks for sharing, see, we still need to update the prop-types :p.

[mngu]

@oliviertassinari I cannot see a way to avoid updating them :/
Though the main benefit here is that we don't have to augment each component to use a custom palette.

[oliviertassinari]

the main benefit here is that we don't have to augment each component to use a custom palette.

@mngu Ok, no objections from my side. cc @mui-org/core in case somebody is against this change or want to challenge it.

[mnajdova]

I like the idea of inverting the logic, as in my opinion using all values of the palette may be the most common use-case (if I am wrong in this assumption then it doesn't make sense that we do the changes). If we do this change, that means that only the more advanced scenarios will require module augmentation (wanting to restrict some colors). No objection from my side, we just have to make sure to nicely document this breaking change.

[oliviertassinari]

It seems that we have a working POC in #24778 (comment) and we have a product direction agreement. I'm changing the label :)

[sakura90]

@oliviertassinari I would like to work on this issue and am new to this repository. May you be my reviewer when I submit a PR?

[oliviertassinari]

@sakura90 I will likely assist in getting this change merged yes. @siriwatknp has been spending time on improving the color management system, so he will likely review it too.

[siriwatknp]

@oliviertassinari @mnajdova What do you think about,

1. fix all the type issue for the components already support all color palette (eg. Chip, Badge, ...a lot more).
2. improve some component to support all color palette (eg. Fab)
3. work on color extension suggested by this comment

For 1 and 2, we can start separately now since (1) is bug fix and (2) is enhancement.
For 3, wait for someone to pick up or we can work on this in v5 beta? (is it a breaking change?)

[mnajdova]

For 1 and 2, we can start separately now since (1) is bug fix and (2) is enhancement.

👍

[oliviertassinari]

The plan sounds great. The outcome will likely make many developers happy. I have added it to the v5.1 milestone to communicate that we want to get it in sooner rather than later.

[manorinfinity]

I can see the tasks for this issue are already merged and closed. @oliviertassinari does this issue has any other pending task? This is my first time contributing and I found this issue in good first issue. But don't understand what actually needs to be done here. Can you help me to understand?

[oliviertassinari]

What we might miss is the ability to easily add new colors, but I'm not the best person to ask.

[siriwatknp]

@manorinfinity @oliviertassinari From my comment, what's left is (3) but we could split it into another issue since it is related to color extension.

I think we can close this issue.

[davidcardd1]

Hello! I'm new with contributing to Open Source and I would like to help with the color extension task. Is there an issue related to this already?

[sreeragdas]

hey can i work on this

[Mbistami]

If no one working on this can I be assigned to this issue please?! @eps1lon

[Mbistami]

@oliviertassinari can I handle this?