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

[Button] success color #21958

Closed
wants to merge 2 commits into from
Closed

Conversation

Alexey-Sachko
Copy link

@Alexey-Sachko Alexey-Sachko commented Jul 27, 2020

@Alexey-Sachko Alexey-Sachko changed the title Button success color [Button] success color Jul 27, 2020
@Alexey-Sachko
Copy link
Author

I have added "success" for prop "color".
I want to add more colors like "error", "info", "warning".
Should I add them to this PR or may be another PR ?

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this would be solved by #21648. If not, could you explain what's missing from #21648?

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 27, 2020

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 color prop has a structural difference with the variant prop: the desired outcome, from a developer/designer perspective, is much more predictable than with a variant. We can jump ahead, and provide CSS that binds the design tokens and the props directly, bypassing the intermediary step of writing new CSS.
It's why we have kept a different issue open, so far: #13875, it seems to be even more popular (but less generic) than the variant one: #15573.

@eps1lon
Copy link
Member

eps1lon commented Jul 27, 2020

So we add yet another API to do the same thing? Wrapper component, theme.variants and then another one?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 27, 2020
@mnajdova
Copy link
Member

mnajdova commented Jul 27, 2020

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.

@oliviertassinari
Copy link
Member

Closing for #13875, as we will push in the direction highlighted by @mnajdova in #21958 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants