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

Broken Radio if null passed #14696

Closed
koutsenko opened this issue Feb 28, 2019 · 15 comments
Closed

Broken Radio if null passed #14696

koutsenko opened this issue Feb 28, 2019 · 15 comments
Labels
component: radio This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@koutsenko
Copy link

koutsenko commented Feb 28, 2019

  1. Take an example from https://material-ui.com/demos/selection-controls/#radio-buttons
  2. Declare state = { value: null }
  3. Add componentDidMount with this.setState({ value: 'female' })
  4. Run it.

No "Female" option will be chosen.
If value was set to null programmatically, then setting it to another (programmatically) not work.

@oliviertassinari
Copy link
Member

@koutsenko You need to choose between the controlled and uncontrolled behavior. Check the warning in the console.

@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Feb 28, 2019
@koutsenko
Copy link
Author

Ok, but how can i declare controlled-way that no one value is selected?

@koutsenko
Copy link
Author

koutsenko commented Feb 28, 2019

@oliviertassinari And i have no warnings in console.

@oliviertassinari oliviertassinari removed the status: waiting for author Issue with insufficient information label Feb 28, 2019
@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Feb 28, 2019
@oliviertassinari
Copy link
Member

@koutsenko You can set an empty string. I can confirm that no warning is raised.

@oliviertassinari oliviertassinari added component: radio This is the name of the generic UI component, not the React module! and removed status: waiting for author Issue with insufficient information labels Feb 28, 2019
@oliviertassinari
Copy link
Member

@koutsenko This problem only surfaces now because of #13915. We used to not support the uncontrolled radio mode. The best path forward I can think of is to reproduce the React warnings:
https://github.com/facebook/react/blob/c954efa70f44a44be9c33c60c57f87bea6f40a10/packages/react-dom/src/client/ReactDOMInput.js#L148

@koutsenko
Copy link
Author

@oliviertassinari you knows better 👍

Now after all I think that (just Javascript, no library-related rules):

  • null has meaning "no choise was performed" (uncontrolled)
  • undefined means "no value was selected"
  • something else - chosen radio is selected.

I agree that null must raise warning and can be issue solution.

Btw, you said, "no value" must be represented by empty string.
Why so? I just want to understand. Thanks.

@koutsenko
Copy link
Author

Oops.. No, seems it's all wrong:

null is an assigned value. It means nothing.
undefined typically means a variable has been declared but not defined yet.

So, "uncontrolled" means if values are undefined, e.g. not defined in React lifecycles.
But I explicitly set it as null, and explicitly tried to set it to smthing another, and it didnt worked.

Maybe all that case is about "controlled" behaviour?

@oliviertassinari oliviertassinari added new feature New feature or request good first issue Great for first contributions. Enable to learn the contribution process. labels Mar 2, 2019
@oliviertassinari
Copy link
Member

@koutsenko Here is how React handles the controlled/uncontrolled logic:
https://github.com/facebook/react/blob/c954efa70f44a44be9c33c60c57f87bea6f40a10/packages/react-dom/src/client/ReactDOMInput.js#L37-L40
But maybe we could do an exception for the radio group component, accept null as a controlled value 🤔. @eps1lon What do you think about this change?

@eps1lon
Copy link
Member

eps1lon commented Mar 2, 2019

How do we handle ToggleButtonGroup or Select?

@oliviertassinari
Copy link
Member

@eps1lon I should have looked at that, great answer! The ToggleButtonGroup component can only be controlled. For the Select component, React seems to encourage '':

Warning: value prop on select should not be null. Consider using an empty string to clear the component or undefined for uncontrolled components.

@eps1lon
Copy link
Member

eps1lon commented Mar 2, 2019

I would go with that option then (including the warning). I think it's important that devs familiar with react don't encounter different behavior.

@manonthemat
Copy link
Contributor

Does this mean that the RadioGroup should never be controlled?

@joshwooding
Copy link
Member

joshwooding commented Mar 14, 2019

Does this mean that the RadioGroup should never be controlled?

@manonthemat It shouldn't switch between an uncontrolled and controlled state.

@mbrookes
Copy link
Member

Ok, but how can i declare controlled-way that no one value is selected?

That's an incorrect use of radio buttons. Radio buttons should always have one value selected – a default, or that chosen by the user to override the default. If you need "none" as one of the selections, you should add it.

If you're old enough to remember vintage radios (or TVs) with radio buttons, you could force them into an invalid state by gently pressing a button until it disengaged the selected button, but not hard enough to engage the pressed button's latch. The result? No channel tuned (static). Not very exciting, but that's effectively what you're asking for.

@oliviertassinari
Copy link
Member

@mbrookes I'm reopening. I think that we should work when people switch between uncontrolled and controlled behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: radio This is the name of the generic UI component, not the React module! good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants