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

[Switch][Radio][Checkbox] Improve specification compliance #15097

Merged
merged 2 commits into from
Mar 31, 2019

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 28, 2019

Closes #13799. We don't have a good solution for full spec compliance. Handling this case would make the implementation significantly more complex for a low ROI.


  • The hover ripple color should match the color of the selection control.
  • The ripple width is 6px smaller.
  • The ripple should be behind the selection control element.

https://material.io/design/components/selection-controls.html#checkboxes

It was the occasion for me to dive into the customization of this component. Well boy, it's hard! I have refactored the implementation to make it easier to override the styles. I have renamed the class names to match the specification wording:

Capture d’écran 2019-03-27 à 18 35 16

thumb instead of icon, track instead of bar.

@oliviertassinari oliviertassinari added design: material This is about Material Design, please involve a visual or UX designer in the process 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 Mar 28, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 28, 2019

Details of bundle changes.

Comparing: 2c2075e...3d263ef

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core 0.00% -0.02% 350,279 350,282 89,995 89,974
@material-ui/core/Paper 0.00% 0.00% 67,853 67,853 19,820 19,820
@material-ui/core/Paper.esm 0.00% +0.01% 🔺 60,184 60,184 18,562 18,563
@material-ui/core/Popper 0.00% 0.00% 30,463 30,463 10,530 10,530
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 17,364 17,364 5,726 5,726
@material-ui/core/useMediaQuery 0.00% 0.00% 2,469 2,469 1,041 1,041
@material-ui/lab 0.00% -0.00% 148,259 148,259 43,564 43,562
@material-ui/styles 0.00% 0.00% 53,105 53,105 15,434 15,434
@material-ui/system 0.00% 0.00% 17,136 17,136 4,525 4,525
Button 0.00% +0.01% 🔺 87,927 87,927 26,052 26,054
Modal 0.00% 0.00% 82,035 82,035 24,551 24,552
colorManipulator 0.00% 0.00% 3,232 3,232 1,299 1,299
docs.landing 0.00% 0.00% 49,899 49,899 10,803 10,803
docs.main -0.00% 0.00% 645,204 645,189 200,433 200,434
packages/material-ui/build/umd/material-ui.production.min.js +0.01% 🔺 -0.06% 298,576 298,595 82,774 82,724

Generated by 🚫 dangerJS against 3d263ef

@oliviertassinari oliviertassinari force-pushed the selection-control-v4 branch 2 times, most recently from 34bb1b5 to 4228d27 Compare March 28, 2019 15:17
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.

Closes #13799. I'm not handling it.

I don't understand that part. If you just close it without handling it you shouldn't reference it in a PR. This will appear in the issue as the PR that fixed the issue.

The box shadow has different color depending on the state. It is grey when not checked and primary/secondary when checked. Is this intended for a later PR or would this add to much implementation and that's why it is ignored?

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.

Closes #13799. I'm not handling it.

I don't understand that part. If you just close it without handling it you shouldn't reference it in a PR. This will appear in the issue as the PR that fixed the issue.

The box shadow has different color depending on the state. It is grey when not checked and primary/secondary when checked. Is this intended for a later PR or would this add to much implementation and that's why it is ignored?

@oliviertassinari
Copy link
Member Author

I don't understand that part. If you just close it without handling it you shouldn't reference it in a PR. This will appear in the issue as the PR that fixed the issue.

@eps1lon I'm suggesting a won't fix issue label. Supporting it would make the overrides a "hot mess", meaning, I think that we should sacrifice a bit of specification conformance (people won't notice) for a much simpler override story (more important).

@eps1lon
Copy link
Member

eps1lon commented Mar 30, 2019

I was not arguing against any of this. I know our roadmap. I have a problem with referencing an issue from a PR if that PR does not fix the issue.

We can close the issue saying that we don't have a good solution for full spec compliance. But saying that this PR matches the spec and closing the issue is incorrect.

@eps1lon eps1lon changed the title [Switch][Radio][Checkbox] Update to match the specification [Switch][Radio][Checkbox] Improve specification compliance Mar 30, 2019
@oliviertassinari
Copy link
Member Author

@eps1lon I agree, it wasn't my intention.

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! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants