-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Button] success color #21958
[Button] success color #21958
Conversation
Alexey-Sachko
commented
Jul 27, 2020
•
edited
Loading
edited
- I have followed (at least) the PR section of the contributing guide.
I have added "success" for prop "color". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This effort makes me think of #20001, #17475, and #19164. Is #21648 meant to solve the color problem? Probably not. We have started to chat about it with @mnajdova in oliviertassinari#9. The |
So we add yet another API to do the same thing? Wrapper component, theme.variants and then another one? |
It's possible to add new colors with the variants API, but after some digging into it, we decided with @oliviertassinari that we probably want to offer better support for this by allowing dynamic styles based on prop, for example based on the color prop, provide different key from the theme's palette. Then if users add new colors in the palette we could automatically support those colors. Something like: // styles applied when variant="contained"
contained: {
background: theme.palette[props.color].main,
color: theme.palette[props.color].contrastText,
} This should not add new API, it should be available as an option directly in the styles of the component. |
Closing for #13875, as we will push in the direction highlighted by @mnajdova in #21958 (comment). |