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

[RadioGroup] Make value accept any #17132

Merged
merged 5 commits into from
Aug 25, 2019
Merged

[RadioGroup] Make value accept any #17132

merged 5 commits into from
Aug 25, 2019

Conversation

cmeeren
Copy link
Contributor

@cmeeren cmeeren commented Aug 24, 2019

Fixes #17129

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 24, 2019 •
edited
Loading

No bundle size changes comparing 438d435...a1b1393

Generated by 🚫 dangerJS against a1b1393

@oliviertassinari oliviertassinari changed the title [RadioGroup] Make RadioGroup.value accept any [RadioGroup] Make value accept any Aug 24, 2019
@oliviertassinari oliviertassinari added the new feature New feature or request label Aug 24, 2019
@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019 •
edited
Loading

Hold on a sec, should the signature of RadioGroup.onChange be updated too so that the target value is any?

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 24, 2019 •
edited
Loading

@cmeeren I'm having second thoughts about it. I'm ready #12919 to better understand the best course of action.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019

Ok, I'm confused, but ping me there if you need me

@oliviertassinari
Copy link
Member

@cmeeren What do you think of the latest changes?

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019

Fantastic, great job! Hoping this can get published soon so I can update my Fable bindings :) (They are generated from the live HTML docs)

oliviertassinari
oliviertassinari reviewed Aug 24, 2019
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 have marked the relevance changes with ⬆️ .

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019 •
edited
Loading

A possible improvement to this PR is that currently, there's a lot of onX API docs saying that the value can be retrieved using event.target.value, but many of the handlers have this value directly accessible as the second argument to the handler. It would be good to clarify whether these two values are in fact identical.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019

Also perhaps include FormControlLabel, should be the same as Checkbox/Switch/Radio

@oliviertassinari
Copy link
Member

Thanks for the feedback, I have added the missing comments and undocument the second argument when a native alternative is available.

@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019

Thanks! I'll have a look.

cmeeren
cmeeren commented Aug 24, 2019
View reviewed changes
@cmeeren
Copy link
Contributor Author

cmeeren commented Aug 24, 2019

Looks good to me 👍

@oliviertassinari oliviertassinari merged commit 2df26ad into mui:master Aug 25, 2019
@oliviertassinari oliviertassinari added component: radio This is the name of the generic UI component, not the React module! and removed component: RadioGroup labels Mar 31, 2020
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! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Radio] Docs - should 'value' only be string?
3 participants
@cmeeren @mui-pr-bot @oliviertassinari