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

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

Open
2 tasks done
xiaoyu-tamu opened this issue Feb 5, 2021 · 25 comments
Open
2 tasks done

Comments

@xiaoyu-tamu
Copy link
Task lists! Give feedback
Contributor

@xiaoyu-tamu xiaoyu-tamu commented Feb 5, 2021

  • 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
Copy link
Member

@eps1lon eps1lon commented Feb 5, 2021

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.

@xiaoyu-tamu
Copy link
Contributor Author

@xiaoyu-tamu xiaoyu-tamu commented Feb 5, 2021

image

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
Copy link
Contributor

@povilass povilass commented Feb 6, 2021

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;
  }
}
@xiaoyu-tamu
Copy link
Contributor Author

@xiaoyu-tamu xiaoyu-tamu commented Feb 6, 2021

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
Copy link
Contributor

@povilass povilass commented Feb 6, 2021

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

image

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

image

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 6, 2021

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
@oliviertassinari oliviertassinari changed the title [Typescript] Button,Badge add error, success, warning, info to color prop . [design-system] Add default support for error, success, warning, info to color prop Feb 6, 2021
@oliviertassinari oliviertassinari changed the title [design-system] Add default support for error, success, warning, info to color prop [design-system] Support error|success|warning|info by default in color prop Feb 6, 2021
@povilass
Copy link
Contributor

@povilass povilass commented Feb 7, 2021

@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]
>;
  1. 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.

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

Playground

image

image

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
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 7, 2021

@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
Copy link
Contributor

@povilass povilass commented Feb 7, 2021

@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
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 9, 2021

@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:

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

@povilass
Copy link
Contributor

@povilass povilass commented Feb 9, 2021

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

@mngu
Copy link
Contributor

@mngu mngu commented Mar 5, 2021

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
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 5, 2021

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
Copy link
Contributor

@povilass povilass commented Mar 5, 2021

@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
Copy link
Contributor

@mngu mngu commented Mar 5, 2021

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

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 5, 2021

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

@mngu
Copy link
Contributor

@mngu mngu commented Mar 5, 2021

@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
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 5, 2021

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
Copy link
Member

@mnajdova mnajdova commented Apr 1, 2021

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
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 2, 2021

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

@sakura90
Copy link
Contributor

@sakura90 sakura90 commented May 31, 2021

@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
Copy link
Member

@oliviertassinari oliviertassinari commented May 31, 2021

@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
Copy link
Member

@siriwatknp siriwatknp commented Jun 10, 2021

@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
Copy link
Member

@mnajdova mnajdova commented Jun 10, 2021

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

👍

@oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 10, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants