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

[Radio] Migrate RadioButtonIcon to emotion #26068

Merged
merged 5 commits into from
Apr 30, 2021
Merged

[Radio] Migrate RadioButtonIcon to emotion #26068

merged 5 commits into from
Apr 30, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Apr 30, 2021 •
edited

Seems like this is the last component from @material-ui/core that uses the withStyles API.

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 30, 2021 •
edited

Details of bundle changes

Generated by 🚫 dangerJS against b7274ac

oliviertassinari
oliviertassinari reviewed Apr 30, 2021
View reviewed changes
packages/material-ui/src/Radio/RadioButtonIcon.js Outdated
<RadioButtonUncheckedIcon fontSize={fontSize} className={classes.background} />
<RadioButtonCheckedIcon fontSize={fontSize} className={classes.dot} />
</span>
<RadioButtonIconRoot className={clsx(classes.root, { 'Mui-checked': checked })}>
Copy link
Member

Choose a reason for hiding this comment

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

Does it need to use .Mui-checked? It seems to be a private component that could leverage styleProps.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't sure whether is fine to change the specificity... In that case, we can add the style in the slot where it is required and we won't bump the specificity at all in the styles definition

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to depend on styleProps

Copy link
Member

Choose a reason for hiding this comment

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

Good point, maybe it will conflict with inheritance

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't spot any issues, updated to depend on styleProps

@oliviertassinari oliviertassinari added the component: radio This is the name of the generic UI component, not the React module! label Apr 30, 2021
oliviertassinari
oliviertassinari approved these changes Apr 30, 2021
View reviewed changes
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.

I could notice a .Mui-checked used in https://deploy-preview-26068--material-ui.netlify.app/components/radio-buttons/#color but it has no implications

@mnajdova mnajdova merged commit a088c6e into mui:next Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@mnajdova @mui-pr-bot @oliviertassinari