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

[Checkbox, Radio, Switch] Change default color, add color prop #10138

Merged
merged 3 commits into from Feb 13, 2018
Merged

[Checkbox, Radio, Switch] Change default color, add color prop #10138

merged 3 commits into from Feb 13, 2018

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Feb 2, 2018

Closes #4706

Breaking change

The Material Design specification says that selection controls elements should use the application's secondary color.

capture d ecran 2018-02-13 a 19 46 09

-<Checkbox />
-<Switch />
-<Radio />
+<Checkbox color="primary" />
+<Switch color="primary" />
+<Radio color="primary" />

@mbrookes mbrookes added breaking change component: checkbox This is the name of the generic UI component, not the React module! component: switch This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! labels Feb 2, 2018
@oliviertassinari oliviertassinari self-assigned this Feb 3, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 3, 2018

@mbrookes What about applying #9742 (comment) here and everywhere else?

  • Benchmarking what Bootstrap is doing, I have noticed that they use a specificity of 1 for a variant and increase it to 2 for a variant modifier (disabled, hover, focus, checked, etc). I think it's smart. It can make the override of our components simpler.
  • Same story on Ant design side.

@oliviertassinari oliviertassinari removed their assignment Feb 3, 2018
@mbrookes
Copy link
Member Author

mbrookes commented Feb 4, 2018

Sure, I'll do it here. Doing it everywhere else is for another PR! 😰

@oliviertassinari
Copy link
Member

Doing it everywhere else is for another PR!

@mbrookes Yes, it's! What's important is to decide on the CSS specificity approach.

@mbrookes
Copy link
Member Author

mbrookes commented Feb 5, 2018

@oliviertassinari Done.

@oliviertassinari oliviertassinari self-assigned this Feb 5, 2018
@oliviertassinari oliviertassinari added the on hold There is a blocker, we need to wait label Feb 5, 2018
@oliviertassinari oliviertassinari removed their assignment Feb 6, 2018
@mbrookes mbrookes removed the on hold There is a blocker, we need to wait label Feb 11, 2018
@oliviertassinari oliviertassinari self-assigned this Feb 11, 2018
@oliviertassinari oliviertassinari added this to the v1.0.0-prerelease milestone Feb 11, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2018

This is a pilot toward solving #10010 and #9742 at scale. It's inspired by the Bootstrap approach to write CSS. Basically, it's following two rules:

  1. A variant has a one level specificity. People override can be considered a variant. So we can keep things as simple as possible.
  2. We increase the specificity for a variant modifier. We already have to do it for the pseudo-classes (:hover, :focus, etc.). It's allowing much more control at the cost of an extra complexity. Hopefully, it's making it more intuitive to our users.
const styles = {
  root: {
    color: green[600],
    '&$checked': {
      color: green[500],
    },
  },
  checked: {},
};

@mbrookes
Copy link
Member Author

mbrookes commented Feb 13, 2018

@oliviertassinari It doesn't look like you started with the latest version of this PR, as we have lost some of the docs changes. I can force push to my branch so you can rebase your commit.

@mbrookes
Copy link
Member Author

mbrookes commented Feb 13, 2018

Alternatively we can omit the specificity change in my last commit (leaving the docs changes), merge this PR, and you can submit a separate PR for the specificity pilot. (What we should probably have done in the first place.)

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2018

@mbrookes I'm sorry If I have lost some changes you did. Yes, please submit them back :).

merge this PR, and you can submit a separate PR for the specificity pilot. (What we should probably have done in the first place.)

So the plan is to limit the scope of this pull-request to the selection control color breaking change and I will push the specificity change in another pull-request targeting the tabs and the selection control components? (I don't want to touch the button before @nathanmarks effort on this component)
Sounds good to me.

@mbrookes
Copy link
Member Author

Okay, I've removed the specificity change from the last commit & force pushed.

I know you had added an unlabelled Checkboxes example in one of your commits, but I'd had the opposite thought, as the unlabelled radios & switches don't really make much sense. I"m not sure what the anticipated use case might be; in a table perhaps? If so, that's probably what the demo should show. As for the unlabelled switches example, that contradicts the text before it:

"The option that the switch controls, as well as the state it’s in, should be made clear from the corresponding inline label."

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2018

as the unlabelled radios & switches don't really make much sense

@mbrookes No it doesn't. But I think it's too much assumption that people are going to use our label helper. For instance, Open Collective isn't. Better showing them the simplest example possible first (unlabeled).

@mbrookes
Copy link
Member Author

mbrookes commented Feb 13, 2018

In that case the ordering of the examples as it is for Radio makes most sense, with the unlabelled selection control being a minimal example. I'd still prefer to see them in the context of a use-case, but it isn't a priority.

@oliviertassinari oliviertassinari merged commit bfbf4ed into mui:v1-beta Feb 13, 2018
@oliviertassinari oliviertassinari changed the title [Selection Controls] Change default color, add color prop [Checkbox, Radio, Switch] Change default color, add color prop Feb 17, 2018
@gabrielliwerant
Copy link

gabrielliwerant commented Feb 20, 2018

@mbrookes Just curious, but if the spec says to use the secondary color, why is it being set to the primary color?

@oliviertassinari
Copy link
Member

@gabrielliwerant The diff shows the upgrade path to keep the previous behavior. But if you are happy with the change. No need to do anything.

@mbrookes mbrookes deleted the selection-add-color-prop branch September 11, 2018 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: checkbox This is the name of the generic UI component, not the React module! component: radio This is the name of the generic UI component, not the React module! component: switch This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants