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] Add missing color type #26593

Merged
merged 3 commits into from Jun 10, 2021
Merged

Conversation

sakura90
Copy link
Contributor

@sakura90 sakura90 commented Jun 4, 2021

One step toward #24778.

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 4, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against b48e204

@sakura90 sakura90 marked this pull request as ready for review June 4, 2021 14:32
@sakura90
Copy link
Contributor Author

sakura90 commented Jun 6, 2021

@oliviertassinari @siriwatknp could you review the commit? Today is a weekend, so almost everyone is taking the time off. Please review when you get time.

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.

This is a step in the direction I think we should go into 👍. However, I think that we could push it much further:

  1. We can extend most of the other component to allow all the default colors of the theme
  2. We can extract the colors from the theme. See [design-system] Support error|success|warning|info by default in color prop #24778 (comment) and this quick test (PaletteColorKeys contains neutral). It would simplify this part of the docs: https://next.material-ui.com/customization/palette/#adding-new-colors.

@oliviertassinari oliviertassinari added the design This is about UI or UX design, please involve a designer label Jun 6, 2021
@oliviertassinari oliviertassinari changed the title [design-system] Support error|success|warning|info by default in color prop in Button [design-system] Extend support of default colors Jun 6, 2021
@sakura90
Copy link
Contributor Author

sakura90 commented Jun 7, 2021

We can extend most of the other component to allow all the default colors of the theme

@oliviertassinari Bootstrap v4/5 supports Light/Dark/Link colors. Nice if Material-UI can match as many colors as Bootstrap to be feature color complete. What is your opinion of supporting more colors?

https://getbootstrap.com/docs/4.6/components/buttons/#examples

@oliviertassinari
Copy link
Member

What is your opinion of supporting more colors?

@sakura90 Off-topic to these changes. Here, I think that we should focus on the support the existing ones of the theme in components 1. , with a possible extension to 2. if it's not too complex/or make 1. easier. But to answer, this is up to @danilo-leal.

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

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 8, 2021
@siriwatknp siriwatknp added component: button This is the name of the generic UI component, not the React module! typescript bug 🐛 Something doesn't work labels Jun 8, 2021
@sakura90
Copy link
Contributor Author

sakura90 commented Jun 8, 2021

This is a step in the direction I think we should go into 👍. However, I think that we could push it much further:

We can extend most of the other component to allow all the default colors of the theme
We can extract the colors from the theme. See #24778 (comment) and this quick test (PaletteColorKeys contains neutral). It > > would simplify this part of the docs: https://next.material-ui.com/customization/palette/#adding-new-colors.

Have some private things to do, so I am going to be unavailable for atleast about a week. Please feel free to continue the work. The next change is going to address the above step. I am going to continue working on the above step when I am back if nobody works on it.

@siriwatknp siriwatknp changed the title [design-system] Extend support of default colors [Button] Add missing color type Jun 9, 2021
@siriwatknp
Copy link
Member

I think we should merge this since it is a bug fix for Button (so it does not block beta) and open another PR to work on the color design-system.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 9, 2021

since it is a bug fix for Button

@siriwatknp I would personally not frame the current behavior as a bug as we have been pushing against it in v4 to restrict the number of colors to make it consistent with the other components and do not have the JSS class name grow significantly.

Happy to move with a smaller step, and merge. We know we want to support all the theme.palette colors in all the components. It will require a lot more effort but it's a step forward.

@siriwatknp
Copy link
Member

siriwatknp commented Jun 10, 2021

since it is a bug fix for Button

@siriwatknp I would personally not frame the current behavior as a bug as we have been pushing against it in v4 to restrict the number of colors to make it consistent with the other components and do not have the JSS class name grow significantly.

Happy to move with a smaller step, and merge. We know we want to support all the theme.palette colors in all the components. It will require a lot more effort but it's a step forward.

I consider it as typescript bug because

// @ts-expect-error
<Button color="success" />

the button is green.

What if we merge this and I will open another PR to list all the components that need to be updated and other people can help?

Anyway, feel free to change the title and label back. I think this is one step toward #24778.

@siriwatknp siriwatknp merged commit d36858d into mui:next Jun 10, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 10, 2021

I consider it as typescript bug because

@siriwatknp Interesting, to the limit where we don't document it.

What if we merge this and I will open another PR to list all the components that need to be updated and other people can help?

Is #24778 enough to provide more guidance for the community?

@siriwatknp
Copy link
Member

Is #24778 enough to provide more guidance for the community?

Yep, I think it is enough.

@sakura90 sakura90 deleted the color-prop-default branch June 14, 2021 10:46
@oliviertassinari oliviertassinari removed the design This is about UI or UX design, please involve a designer label May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: button This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants