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.

Sign up for GitHub

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

Jump to bottom

[ToggleButton] Add color palette types #27046

Merged
merged 2 commits into from
Jul 6, 2021
Merged

[ToggleButton] Add color palette types #27046

merged 2 commits into from
Jul 6, 2021

Conversation

ShirasawaSama
Copy link
Contributor

@ShirasawaSama ShirasawaSama commented Jul 1, 2021 •
edited by oliviertassinari
Loading

In 5.0.0-alpha.38, if I use <ToggleButtonGroup color='error' />, it will report:

Warning: Failed prop type: Invalid prop `color` of value `error` supplied to `ForwardRef(ToggleButtonGroup1)`, expected one of ["primary","secondary","standard"].

But it still works.

Preview: https://deploy-preview-27046--material-ui.netlify.app/api/toggle-button/#props

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 1, 2021 •
edited
Loading

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 04b2903

@github-actions github-actions bot mentioned this pull request Jul 1, 2021
Closed
@mnajdova mnajdova requested a review from siriwatknp July 1, 2021 10:17
@mnajdova
Copy link
Member

mnajdova commented Jul 1, 2021

@siriwatknp had a list of which components should support these colors, he could provide more information/guidance

@siriwatknp
Copy link
Member

@ShirasawaSama the details is in this PR #26697. We tried to add color type for the components that make sense. I am happy to learn about your use case, could you share how your use case on the ToggleButton? (screenshot is fine).

If there is no objection for the ToggleButton to support color palette, we can move forward with adding it.

@ShirasawaSama
Copy link
Contributor Author

ShirasawaSama commented Jul 1, 2021 •
edited
Loading

@siriwatknp

image
image
image

This may be against the design rules of material design.

I have another question. Why do some components have different default color values, such as:

  • ToggleButtonGroup: standard
  • ButtonGroup: inherit
  • Checkbox: default

This makes me confused.

@siriwatknp
Copy link
Member

Seems like you change the wrong component, should it be ToggleButton instead of ButtonGroup?

@siriwatknp siriwatknp mentioned this pull request Jul 4, 2021
Merged
1 task
@ShirasawaSama
Copy link
Contributor Author

ShirasawaSama commented Jul 4, 2021 •
edited
Loading

@siriwatknp Maybe I should change the tag in the title to [core] (Or split into two PRs), because I modified these two components.

@siriwatknp
Copy link
Member

@ShirasawaSama can you revert the code for ToggleButton (I have open another PR) so this PR contains only ButtonGroup.

by the way, from your use case, do you need ButtonGroup? it seems like only ToggleButton that needs color.

@ShirasawaSama ShirasawaSama force-pushed the fix-the-color-prop-of-button-group branch from 8ebef06 to b7486ed Compare July 5, 2021 10:14
@siriwatknp siriwatknp changed the title [ButtonGroup] Fix the color prop of buttons [ToggleButton] add color palette types Jul 6, 2021
@siriwatknp siriwatknp added component: toggle button This is the name of the generic UI component, not the React module! and removed component: ButtonGroup The React component. labels Jul 6, 2021
@siriwatknp
Copy link
Member

@ShirasawaSama my apology. From your explanation, I will merge only ToggleButton color palette. If you have a use case for ButtonGroup, feel free to open another PR.

siriwatknp
siriwatknp reviewed Jul 6, 2021
View reviewed changes
packages/material-ui/src/ButtonGroup/ButtonGroup.js
@@ -51,7 +51,7 @@ const useUtilityClasses = (styleProps) => {
`grouped${capitalize(orientation)}`,
`grouped${capitalize(variant)}`,
`grouped${capitalize(variant)}${capitalize(orientation)}`,
color !== 'default' && `grouped${capitalize(variant)}${capitalize(color)}`,
`grouped${capitalize(variant)}${capitalize(color)}`,
Copy link
Member

Choose a reason for hiding this comment

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

color will never be "default", the change is better.

@siriwatknp siriwatknp merged commit 619eac0 into mui:next Jul 6, 2021
@oliviertassinari oliviertassinari changed the title [ToggleButton] add color palette types [ToggleButton] Add color palette types Jul 7, 2021
@oliviertassinari oliviertassinari mentioned this pull request Jul 7, 2021
Merged
@ShirasawaSama ShirasawaSama deleted the fix-the-color-prop-of-button-group branch July 10, 2021 09:11
@siriwatknp siriwatknp mentioned this pull request Jul 19, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@siriwatknp siriwatknp siriwatknp approved these changes

@eps1lon eps1lon eps1lon approved these changes

Assignees
No one assigned
Labels
component: toggle button This is the name of the generic UI component, not the React module! typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ShirasawaSama @mui-pr-bot @mnajdova @siriwatknp @eps1lon