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] Support uncontrolled mode #13915

Closed
2 tasks done
Slessi opened this issue Dec 15, 2018 · 6 comments
Closed
2 tasks done

[Radio] Support uncontrolled mode #13915

Slessi opened this issue Dec 15, 2018 · 6 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

@Slessi
Copy link
Contributor

Slessi commented Dec 15, 2018

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Clicking radio button should select it

Current Behavior 😯

Clicking radio button does not select it

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/2pw922z7xn

import React from "react";
import {
  FormControl,
  FormLabel,
  RadioGroup,
  Radio,
  FormControlLabel
} from "@material-ui/core";

export default _ => (
  <FormControl>
    <FormLabel>Sex</FormLabel>

    <RadioGroup name="sex">
      <FormControlLabel value="MALE" control={<Radio />} label="Male" />

      <FormControlLabel value="FEMALE" control={<Radio />} label="Female" />
    </RadioGroup>
  </FormControl>
);

Context 🔦

I want an uncontrolled Radio group

Your Environment 🌎

Tech Version
Material-UI v3.6.2
React v16.6.3
Browser Chrome
@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Dec 15, 2018
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 15, 2018

@Slessi The radio component needs to be controlled somewhere. React only trigger a change event on the radio that receives the new value, nothing happens on the already selected radio. This means that we can't keep track of the selection state individually for each radio, the logic has to be centralized in the radio group component: facebook/react#1471.

What's your use case for an uncontrolled radio group?

@oliviertassinari oliviertassinari added component: radio This is the name of the generic UI component, not the React module! and removed docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Dec 15, 2018
@Slessi
Copy link
Contributor Author

Slessi commented Dec 15, 2018

What's your use case

I'm lazy and didn't feel like writing onChange / value on all fields and setting up state - so I was relying on name attributes and currentTarget of my form's onsubmit event

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 15, 2018

@Slessi Ok, this sounds valid to me. I think that we should implement an uncontrolled logic in the radio group. Do you want to give it a shot?

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

@Slessi What do you think of this change?

--- a/packages/material-ui/src/RadioGroup/RadioGroup.js
+++ b/packages/material-ui/src/RadioGroup/RadioGroup.js
@@ -9,6 +9,15 @@ import { createChainedFunction, find } from '../utils/helpers';
 class RadioGroup extends React.Component {
   radios = [];

+  state = {
+    value: null,
+  };
+
+  constructor(props) {
+    super(props);
+    this.isControlled = props.value != null;
+  }
+
   focus = () => {
     if (!this.radios || !this.radios.length) {
       return;
@@ -30,15 +39,22 @@ class RadioGroup extends React.Component {
     focusRadios[0].focus();
   };

-  handleRadioChange = (event, checked) => {
-    if (checked && this.props.onChange) {
+  handleChange = event => {
+    if (!this.isControlled) {
+      this.setState({
+        value: event.target.value,
+      });
+    }
+
+    if (this.props.onChange) {
       this.props.onChange(event, event.target.value);
     }
   };

   render() {
-    const { children, name, value, onChange, ...other } = this.props;
+    const { children, name, value: valueProp, onChange, ...other } = this.props;

+    const value = this.isControlled ? valueProp : this.state.value;
     this.radios = [];

     return (
@@ -64,7 +80,7 @@ class RadioGroup extends React.Component {
               }
             },
             checked: value === child.props.value,
-            onChange: createChainedFunction(child.props.onChange, this.handleRadioChange),
+            onChange: createChainedFunction(child.props.onChange, this.handleChange),
           });
         })}
       </FormGroup>

@oliviertassinari oliviertassinari changed the title Uncontrolled Radio components don't work [Radio] Support uncontrolled mode Dec 16, 2018
@Slessi
Copy link
Contributor Author

Slessi commented Dec 16, 2018

Looks ok to me - Not sure if Radio component itself would need changing (since docs offer the option of using stand-alone Radio).

Might want to implement that warning the other inputs have too

Warning: A component is changing a controlled input of type text to be uncontrolled. Input elements should not switch from controlled to uncontrolled (or vice versa). Decide between using a controlled or uncontrolled input element for the lifetime of the component

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 16, 2018

Might want to implement that warning the other inputs have too

@Slessi This warning should already be triggered:
https://github.com/mui-org/material-ui/blob/dcbf543401ce2ffa83d1e3276af68a27691732a6/packages/material-ui/src/internal/SwitchBase.test.js#L451

Looks ok to me

Alright, great. Let's leave it here and wait for somebody to submit a pull request :).

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

2 participants