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

[Radio] Fix support for number value type #26772

Merged
merged 10 commits into from
Sep 17, 2021

Conversation

sakura90
Copy link
Contributor

@sakura90 sakura90 commented Jun 15, 2021

Closes #26333

@mui-pr-bot
Copy link

mui-pr-bot commented Jun 15, 2021

Details of bundle changes

Generated by 🚫 dangerJS against b0a9129

@oliviertassinari oliviertassinari changed the title [Radio] Have Radio value and RadioGroup defaultValue accept any type. [Radio] Fix support for any value type Jun 15, 2021
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: radio This is the name of the generic UI component, not the React module! labels Jun 15, 2021
@oliviertassinari
Copy link
Member

Could we add a test case too?

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.

I don't think we should do this. Comparing arrays by their string value does not make much sense. People should explicitly stringify the value if they wish to.

This becomes especially confusing if the type changes over time

@sakura90

This comment has been minimized.

@sakura90
Copy link
Contributor Author

@eps1lon The PR is a draft PR. It is not ready for being reviewed. Please look at the following link:

https://codesandbox.io/s/create-react-app-with-typescript-forked-qlk19?file=/src/App.tsx

After a user clicks Option a1b3, the radio button is not checked, but it should be. I have to fix itn before letting you review it.

I don't think we should do this. Comparing arrays by their string value does not make much sense. People should explicitly stringify the value if they wish to.
This becomes especially confusing if the type changes over time

Please let me comment on it after I fix the above problem. It is not about Material-UI but JavaScript. typeof Array is object, so comparing arrays by their string value is not going to be ther.

image

@oliviertassinari oliviertassinari force-pushed the radio-checked-when-string branch 4 times, most recently from ebc3b65 to 5fc9a7e Compare June 20, 2021 22:19
@oliviertassinari oliviertassinari marked this pull request as ready for review June 20, 2021 22:20
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 20, 2021

I have pushed a new commit with a test case, only to support data structures with a decent .toString(), most notably numbers.

I wasn't too sure if we needed to support rich data structure as object or array:

  • Pros: More flexibility for the developers, allow to be quick and dirty 👌
  • Cons: The value can't be sent to a form with POST (who does this anyway, 5% of the audience?)

#26333 was about numbers, so maybe we can defer this "object" aspect.

@siriwatknp
Copy link
Member

#26333 was about numbers, so maybe we can defer this "object" aspect.

Totally agree, string | number is enough.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 28, 2021
@sakura90
Copy link
Contributor Author

sakura90 commented Aug 9, 2021

@siriwatknp @oliviertassinari Look to me the PR can be closed. I was thinking about supporting any for Radio value. @siriwatknp helped change support to number/string. Support to number/string is enough to resolve #26333, and so I think the change was ok. @oliviertassinari helped add a test case and reorganized the utility function areEqualValues, and I think the PR became better for resolving #26333. The PR currently is in the opne state but it can be closed. There is a button 'Close with comment'. Please let me try to use the button to close it.

@sakura90 sakura90 closed this Aug 9, 2021
@oliviertassinari
Copy link
Member

We were really close, I'm reopening

@sakura90
Copy link
Contributor Author

sakura90 commented Aug 9, 2021

@oliviertassinari could you tell me what else needs to be done? Not following the PR for a while, so not sure what is going on.

@oliviertassinari oliviertassinari force-pushed the radio-checked-when-string branch from 401a416 to 3006f4a Compare August 9, 2021 21:33
@oliviertassinari
Copy link
Member

@sakura90 We only need a rebase, done :)

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 9, 2021
@oliviertassinari oliviertassinari changed the title [Radio] Fix support for any value type [Radio] Fix support for number value type Aug 9, 2021
@oliviertassinari
Copy link
Member

@eps1lon Is #26772 (review) still up to date? The PR now focuses on fixing the support for number values. It works more closely to a native input type="checkbox". What use case did you have in mind with the array?

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021
@eps1lon eps1lon changed the base branch from next to master September 14, 2021 09:15
@eps1lon eps1lon self-assigned this Sep 14, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 14, 2021
@eps1lon eps1lon removed their assignment Sep 14, 2021
@eps1lon eps1lon merged commit 5c00320 into mui:master Sep 17, 2021
@Krasnopir
Copy link

Sorry. But if use Radio with FormControlLabel in RadioGroup - problem remained.
I can set for Radio value as number, but when change, in form state it transform to string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: radio 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.

[Radio] Radio not getting checked when the value is not a string
6 participants