-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[material-ui][RadioGroup] Apply classnames #41610
[material-ui][RadioGroup] Apply classnames #41610
Conversation
Netlify deploy previewhttps://deploy-preview-41610--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
There was a problem hiding this 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 👍🏼
I wouldn't remove the FormGroup classes unless there's an issue with them. I don't think they bloat the component too much. We could open an issue to discuss it if you feel like it's necessary, but as you mention, it would have to remain consistent with other components.
I also included tests.
Alright, I am fine if it needs to be consistent. |
Fixes #41076
Preview: https://deploy-preview-41610--material-ui.netlify.app/material-ui/react-radio-button/
Rather than modifying the radio group classnames in the API documentation, I think the correct fix is to apply those classnames to the Radio Group component. The API documentation for Radio button CSS classes is correct.
Having both
MuiFormGroup
andMuiRadioGroup
classes might not seem ideal, but it's consistent with other components like the Outlined Input, which has bothMuiOutlinedInput
andMuiInputBase
related classnames. However, this does lead to an increase in bundle size.If we decide to retain only the radio group related classes, two things need consideration: