[RadioGroup] Warn for uncontrolled <-> controlled switch #14878

manonthemat · 2019-03-14T00:44:28Z

I have followed (at least) the PR section of the contributing guide.

mui-pr-bot · 2019-03-14T00:49:18Z

Details of bundle changes.

Comparing: bdb4baa...df1a893

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.01% 🔺	+0.01% 🔺	361,335	361,381	92,103	92,110
@material-ui/core/Paper	0.00%	0.00%	68,536	68,536	19,978	19,978
@material-ui/core/Paper.esm	0.00%	0.00%	61,725	61,725	18,888	18,888
@material-ui/core/Popper	0.00%	0.00%	30,458	30,458	10,567	10,567
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,284	17,284	5,700	5,700
@material-ui/core/useMediaQuery	0.00%	0.00%	2,471	2,471	1,038	1,038
@material-ui/lab	0.00%	0.00%	170,524	170,524	49,897	49,897
@material-ui/styles	0.00%	0.00%	53,729	53,729	15,486	15,486
@material-ui/system	0.00%	0.00%	17,041	17,041	4,488	4,488
Button	0.00%	0.00%	90,262	90,262	26,730	26,730
Modal	0.00%	0.00%	88,646	88,646	26,317	26,317
colorManipulator	0.00%	0.00%	3,232	3,232	1,298	1,298
docs.landing	0.00%	0.00%	51,976	51,976	11,324	11,324
docs.main	0.00%	0.00%	651,342	651,342	202,307	202,307
packages/material-ui/build/umd/material-ui.production.min.js	+0.01% 🔺	-0.00%	311,373	311,419	85,482	85,481

Generated by 🚫 dangerJS against df1a893

joshwooding · 2019-03-14T00:50:15Z

This approach won't work, the constructor is only called once and we need to check on prop changes. The check should probably be done in componentDidUpdate.

joshwooding · 2019-03-14T01:07:04Z

Sorry, I wasn’t really clear. IsControlled is set in the constructor and doesn’t change you should do the same check in componentDidUpdate and warn if the result is different from isControlled.

oliviertassinari · 2019-03-14T15:12:06Z

@manonthemat We have recently introduced uncontrolled behavior support for this component. We want to support it. What we are interestd into is to warn when people incorrectly switch between the uncontrolled and controlled mode.

mbrookes · 2019-03-14T18:16:19Z

Since we can't make the RadioGroup defaultValue a required prop, (since it isn't required when RadioGroup is controlled), I suggest we:

Update the defaultValue description to something like:

  /**
-   * The default input value, useful when not controlling the component.
+   * The default input value of an uncontrolled RadioGroup. Required when `RadioGroup` is uncontrolled.
   */
  defaultValue: PropTypes.oneOfType([PropTypes.string, PropTypes.number, PropTypes.bool]),

Warn if RadioGroup is uncontrolled, and defaultValue is not provided.
Optionally we could also warn if value or defaultValue is not valid, i.e. doesn't match the value of a child.

manonthemat · 2019-03-14T18:19:15Z

Thanks for the guidance. I'll give it another shot.

for a trailing comma

packages/material-ui/src/RadioGroup/RadioGroup.js

since changing between controlled and uncontrolled behavior is discouraged

mbrookes · 2019-03-14T19:41:38Z

@manonthemat Hold fire on my suggestion. @oliviertassinari is suggesting (in private chat) that we shouldn't be opinionated. I personally disagree, but this isn't my personal project.

Let's reach a consensus on what to warn and what not before you devote more time to this PR.

oliviertassinari · 2019-03-14T19:52:22Z

packages/material-ui/src/RadioGroup/RadioGroup.js

@@ -96,7 +106,7 @@ RadioGroup.propTypes = {
   */
  children: PropTypes.node,
  /**
-   * The default input value, useful when not controlling the component.
+   * The required default input value of an uncontrolled RadioGroup.


In 90% of the cases, users will be better off with a select radio value, no matter what.

NN/g encourages to always have a selected radio: https://www.nngroup.com/articles/checkboxes-vs-radio-buttons/.

But, it's not something everybody agrees with, e.g. Windows. They recognize that an always selected value should be the norm. However, there are exceptions where not providing a default value is more appropriate:

Exceptions: Don't have a default selection if:

There is no acceptable default option for safety, security, or legal reasons and therefore the user must make an explicit choice. If the user doesn't make a selection, display an error message to force one.

The user interface (UI) must reflect the current state and the option hasn't been set yet. A default value would - incorrectly imply that the user doesn't need to make a selection.

The goal is to collect unbiased data. Default values would bias data collection.

The group of radio buttons represents a property in a mixed state, which happens when displaying a property for multiple objects that don't have the same setting. Don't display an error message in this case since each object has a valid state.

So, I'm in favor of encouraging an always selected radio, but allow people to have zero selected: nothing required, but highly encouraged.

oliviertassinari · 2019-03-14T19:54:35Z

packages/material-ui/src/RadioGroup/RadioGroup.js

@@ -20,6 +20,16 @@ class RadioGroup extends React.Component {
    }
  }

+  componentDidUpdate(_prevProps, prevState) {
+    warning(
+      this.isControlled === (prevState === null),


Something is off here, it makes me think we miss tests.

oliviertassinari

I'm proposing this alternative solution. But we should probably wait on #14585 before merging this one. It will avoid conflict, #14585 doesn't need one.

packages/material-ui/src/RadioGroup/RadioGroup.test.js

mbrookes · 2019-03-18T21:21:15Z

packages/material-ui/src/RadioGroup/RadioGroup.test.js

+      );
+    });
+
+    it('should warn when switching between uncontrolled to controlled', () => {


packages/material-ui/src/RadioGroup/RadioGroup.test.js

packages/material-ui/src/RadioGroup/RadioGroup.js

Co-Authored-By: manonthemat <[email protected]>

warning if RadioGroup is uncontrolled

8a7c382

moved warning into componentDidUpdate

0292dcd

oliviertassinari added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI component: radio This is the name of the generic UI component, not the React module! labels Mar 14, 2019

manonthemat added 2 commits March 14, 2019 11:55

move warning into componentDidUpdate

abcc08e

yarn prettier

4a1b2e4

for a trailing comma

eps1lon reviewed Mar 14, 2019

View reviewed changes

packages/material-ui/src/RadioGroup/RadioGroup.js Outdated Show resolved Hide resolved

manonthemat added 3 commits March 14, 2019 12:23

rename wasControlledOnInit back to isControlled

292c111

since changing between controlled and uncontrolled behavior is discouraged

RadioGroup markdown matching comments in JS file

d592ac6

shortened lines

9994e42

oliviertassinari reviewed Mar 14, 2019

View reviewed changes

oliviertassinari added the new feature New feature or request label Mar 14, 2019

oliviertassinari self-assigned this Mar 15, 2019

oliviertassinari changed the title ~~warning if RadioGroup is uncontrolled~~ [RadioGroup] Warning uncontrolled <-> controlled switch Mar 15, 2019

oliviertassinari changed the title ~~[RadioGroup] Warning uncontrolled <-> controlled switch~~ [RadioGroup] Warn for uncontrolled <-> controlled switch Mar 15, 2019

copy React warning message

3fcf40b

oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Mar 15, 2019

oliviertassinari requested review from joshwooding and mbrookes March 15, 2019 23:10

oliviertassinari approved these changes Mar 15, 2019

View reviewed changes

oliviertassinari assigned oliviertassinari and unassigned oliviertassinari Mar 15, 2019

oliviertassinari added the PR: good for merge label Mar 18, 2019

mbrookes reviewed Mar 18, 2019

View reviewed changes

packages/material-ui/src/RadioGroup/RadioGroup.test.js Outdated Show resolved Hide resolved

mbrookes reviewed Mar 18, 2019

View reviewed changes

packages/material-ui/src/RadioGroup/RadioGroup.test.js Outdated Show resolved Hide resolved

mbrookes reviewed Mar 18, 2019

View reviewed changes

packages/material-ui/src/RadioGroup/RadioGroup.js Outdated Show resolved Hide resolved

mbrookes reviewed Mar 18, 2019

View reviewed changes

packages/material-ui/src/RadioGroup/RadioGroup.js Outdated Show resolved Hide resolved

mbrookes removed the PR: good for merge label Mar 18, 2019

mbrookes and others added 2 commits March 18, 2019 19:02

Update packages/material-ui/src/RadioGroup/RadioGroup.test.js

758f669

Co-Authored-By: manonthemat <[email protected]>

Apply suggestions from code review

df1a893

Co-Authored-By: manonthemat <[email protected]>

oliviertassinari merged commit 7e64283 into mui:next Mar 19, 2019

oliviertassinari mentioned this pull request Apr 10, 2019

RadioGroup does not update when props change #15305

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RadioGroup] Warn for uncontrolled <-> controlled switch #14878

[RadioGroup] Warn for uncontrolled <-> controlled switch #14878

manonthemat commented Mar 14, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Mar 14, 2019 •

edited

Loading

joshwooding commented Mar 14, 2019 •

edited

Loading

joshwooding commented Mar 14, 2019

oliviertassinari commented Mar 14, 2019

mbrookes commented Mar 14, 2019 •

edited

Loading

manonthemat commented Mar 14, 2019

mbrookes commented Mar 14, 2019

oliviertassinari Mar 14, 2019 •

edited

Loading

oliviertassinari Mar 14, 2019 •

edited

Loading

oliviertassinari left a comment

mbrookes Mar 18, 2019

[RadioGroup] Warn for uncontrolled <-> controlled switch #14878

[RadioGroup] Warn for uncontrolled <-> controlled switch #14878

Conversation

manonthemat commented Mar 14, 2019 • edited by oliviertassinari Loading

mui-pr-bot commented Mar 14, 2019 • edited Loading

joshwooding commented Mar 14, 2019 • edited Loading

joshwooding commented Mar 14, 2019

oliviertassinari commented Mar 14, 2019

mbrookes commented Mar 14, 2019 • edited Loading

manonthemat commented Mar 14, 2019

mbrookes commented Mar 14, 2019

oliviertassinari Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

oliviertassinari left a comment

Choose a reason for hiding this comment

mbrookes Mar 18, 2019

Choose a reason for hiding this comment

manonthemat commented Mar 14, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Mar 14, 2019 •

edited

Loading

joshwooding commented Mar 14, 2019 •

edited

Loading

mbrookes commented Mar 14, 2019 •

edited

Loading

oliviertassinari Mar 14, 2019 •

edited

Loading

oliviertassinari Mar 14, 2019 •

edited

Loading