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

[theme] Add base color palette type to components #26697

Merged
merged 41 commits into from
Jun 23, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 11, 2021

first step toward #24778

preview https://deploy-preview-26697--material-ui.netlify.app/

Changes in this PR

For each component

  • add color prop type in ${Component}.js
  • add color type in ${Component}.d.ts
  • add component color demo
  • add <Component color /> to color-palette-prop.spec.tsx

Note: *Classes.ts remain the same.

Components

These are the proposed components that should support color palette in the theme
(primary, secondary, error, info, success, warning).

Likely to appear in Dashboard and used as status

Common

Form controls


*Components that does not support all color palette type

  • AppBar
  • ButtonGroup
  • ToggleButton
  • ToggleButtonGroup
  • Alert
  • Fab
  • ListSubheader
  • PaginationItem
  • Tabs

Once the list is finalized, then fix the types and missing classes for the components.

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 11, 2021

Details of bundle changes (experimental)

@material-ui/lab: parsed: -0.16% 😍, gzip: -0.05% 😍

Generated by 🚫 dangerJS against 697bfec

@eps1lon

This comment has been minimized.

@siriwatknp

This comment has been minimized.

@eps1lon

This comment has been minimized.

@siriwatknp

This comment has been minimized.

@mnajdova

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@mnajdova
Copy link
Member

mnajdova commented Jun 16, 2021

@siriwatknp the list looks solid. I would maybe add the Fab, ToggleButton and Switch components as well as part of the Common/Form controls respectively.

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.

For the color demos, usually, we have only been shown 1 or 2 custom colors. Do we need to show all the colors of the palette? It seems to make the code preview of the demo more challenging to process for the developers. And with live edit #24640, developers will be able to quickly try the value they need. If your intent is visual regression, then I would propose we add custom visual snap (but not in the docs).

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@siriwatknp
Copy link
Member Author

siriwatknp commented Jun 21, 2021

For the color demos, usually, we have only been shown 1 or 2 custom colors. Do we need to show all the colors of the palette? It seems to make the code preview of the demo more challenging to process for the developers. And with live edit #24640, developers will be able to quickly try the value they need. If your intent is visual regression, then I would propose we add custom visual snap (but not in the docs).

My intention is to show that the component support the color in the palette. What if the demo show one more color like success.

<Button color="secondary">
<Button color="success">

@siriwatknp siriwatknp marked this pull request as ready for review June 21, 2021 03:00
@oliviertassinari
Copy link
Member

What if the demo show one more color like success.

We could have two, yeah, and for the button, maybe them all (it's a button, quite a unique component, but not sure it would be better). For the others, I would encourage againt.

In any case, I think that we should always have an inline preview of the demo. If the demo becomes too long preventing it to display (limit around 20 lines). Then it's not flying, and two colors is likely more than enough.

@siriwatknp
Copy link
Member Author

In any case, I think that we should always have an inline preview of the demo. If the demo becomes too long preventing it to display (limit around 20 lines)

I checked, all of the Color demo has inline preview so 2 colors sounds good.

@oliviertassinari oliviertassinari changed the title [Typescript] add base color palette type to components [theme] Add base color palette type to components Jun 21, 2021
@oliviertassinari oliviertassinari added the new feature New feature or request label Jun 21, 2021
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.

Capture d’écran 2021-06-21 à 18 18 54

The contrastText of the success color feels wrong (white works a lot better). One example of where computing contrast is not working 😁. But anyway, it's outside of the scope of this effort. It looks great otherwise.

Capture d’écran 2021-06-21 à 18 20 46

docs/src/pages/components/buttons/IconButtonColors.tsx Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari deleted the branch mui:next June 22, 2021 00:28
@oliviertassinari oliviertassinari changed the base branch from foo to next June 22, 2021 00:46
@siriwatknp
Copy link
Member Author

The contrastText of the success color feels wrong (white works a lot better)

Agree, it will be fixed in #26817 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants