[SwitchBase] Fix ignoring disabled from FormControl #19319

rostislavbobo · 2020-01-20T16:27:45Z

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

Resolves #19288

Resets [Checkbox][Radio][Switch] disabled prop back to undefined

The key point was to support Framer X so that these components keep behaving the same

…[Switch]

mui-pr-bot · 2020-01-20T16:39:10Z

Details of bundle changes.

Comparing: 528a603...b3cd783

bundle	Size Change	Size	Gzip Change	Gzip
@material-ui/core[umd]	▼ -153 B (-0.05% )	317 kB	▼ -27 B (-0.03% )	91.9 kB
@material-ui/core	▼ -153 B (-0.04% )	360 kB	▼ -25 B (-0.03% )	98.7 kB
Radio	▼ -51 B (-0.06% )	84.1 kB	▼ -26 B (-0.10% )	26.5 kB
Switch	▼ -51 B (-0.06% )	82.2 kB	▼ -21 B (-0.08% )	25.9 kB
Checkbox	▼ -51 B (-0.06% )	83 kB	▼ -19 B (-0.07% )	26.3 kB
@material-ui/lab	--	185 kB	--	55.2 kB
@material-ui/styles	--	51.2 kB	--	15.4 kB
@material-ui/system	--	14.8 kB	--	4.07 kB
Alert	--	83.8 kB	--	26.3 kB
AlertTitle	--	64.2 kB	--	20.2 kB
AppBar	--	63.9 kB	--	20 kB
Autocomplete	--	132 kB	--	41.3 kB
Avatar	--	65.2 kB	--	20.6 kB
AvatarGroup	--	62.3 kB	--	19.6 kB
Backdrop	--	67.9 kB	--	21 kB
Badge	--	65.3 kB	--	20.3 kB
BottomNavigation	--	62.4 kB	--	19.6 kB
BottomNavigationAction	--	75.6 kB	--	23.9 kB
Box	--	70.8 kB	--	21.6 kB
Breadcrumbs	--	67.8 kB	--	21.3 kB
Button	--	79.8 kB	--	24.5 kB
ButtonBase	--	74 kB	--	23.3 kB
ButtonGroup	--	83.2 kB	--	25.5 kB
Card	--	62.8 kB	--	19.7 kB
CardActionArea	--	75.1 kB	--	23.7 kB
CardActions	--	62 kB	--	19.5 kB
CardContent	--	62 kB	--	19.4 kB
CardHeader	--	65.1 kB	--	20.5 kB
CardMedia	--	62.3 kB	--	19.6 kB
Chip	--	82.6 kB	--	25.3 kB
CircularProgress	--	64.1 kB	--	20.3 kB
ClickAwayListener	--	3.91 kB	--	1.55 kB
Collapse	--	68 kB	--	21.1 kB
colorManipulator	--	3.88 kB	--	1.52 kB
Container	--	63.2 kB	--	19.8 kB
CssBaseline	--	57.5 kB	--	18.1 kB
Dialog	--	83 kB	--	25.9 kB
DialogActions	--	62.1 kB	--	19.5 kB
DialogContent	--	62.2 kB	--	19.5 kB
DialogContentText	--	64 kB	--	20.1 kB
DialogTitle	--	64.3 kB	--	20.2 kB
Divider	--	62.6 kB	--	19.7 kB
docs.landing	--	50.5 kB	--	13.3 kB
docs.main	--	596 kB	--	194 kB
Drawer	--	84.8 kB	--	25.8 kB
ExpansionPanel	--	72.3 kB	--	22.7 kB
ExpansionPanelActions	--	62.1 kB	--	19.5 kB
ExpansionPanelDetails	--	61.9 kB	--	19.4 kB
ExpansionPanelSummary	--	78.2 kB	--	24.7 kB
Fab	--	76.9 kB	--	24 kB
Fade	--	23.4 kB	--	7.98 kB
FilledInput	--	73.6 kB	--	22.9 kB
FormControl	--	64.4 kB	--	20.1 kB
FormControlLabel	--	65.5 kB	--	20.6 kB
FormGroup	--	62 kB	--	19.5 kB
FormHelperText	--	63.4 kB	--	20 kB
FormLabel	--	63.5 kB	--	19.7 kB
Grid	--	65.1 kB	--	20.4 kB
GridList	--	62.5 kB	--	19.7 kB
GridListTile	--	63.7 kB	--	20 kB
GridListTileBar	--	63.2 kB	--	19.8 kB
Grow	--	24 kB	--	8.19 kB
Hidden	--	65.9 kB	--	20.7 kB
Icon	--	62.8 kB	--	19.7 kB
IconButton	--	76.2 kB	--	23.8 kB
Input	--	72.5 kB	--	22.7 kB
InputAdornment	--	65.1 kB	--	20.5 kB
InputBase	--	70.6 kB	--	22.2 kB
InputLabel	--	65.3 kB	--	20.2 kB
LinearProgress	--	65.3 kB	--	20.4 kB
Link	--	66.6 kB	--	21.1 kB
List	--	62.3 kB	--	19.5 kB
ListItem	--	77.2 kB	--	24.2 kB
ListItemAvatar	--	62.1 kB	--	19.5 kB
ListItemIcon	--	62.2 kB	--	19.5 kB
ListItemSecondaryAction	--	62 kB	--	19.4 kB
ListItemText	--	64.9 kB	--	20.4 kB
ListSubheader	--	62.7 kB	--	19.8 kB
Menu	--	88.8 kB	--	27.4 kB
MenuItem	--	78.2 kB	--	24.5 kB
MenuList	--	66 kB	--	20.7 kB
MobileStepper	--	67.8 kB	--	21.3 kB
Modal	--	14.5 kB	--	5.05 kB
NativeSelect	--	76.8 kB	--	24.3 kB
NoSsr	--	2.19 kB	--	1.04 kB
OutlinedInput	--	74.5 kB	--	23.2 kB
Paper	--	62.3 kB	--	19.5 kB
Popover	--	83.1 kB	--	25.8 kB
Popper	--	28.8 kB	--	10.3 kB
Portal	--	2.92 kB	--	1.3 kB
RadioGroup	--	64.4 kB	--	20 kB
Rating	--	70.5 kB	--	22.7 kB
RootRef	--	4.24 kB	--	1.64 kB
Select	--	116 kB	--	34.6 kB
Skeleton	--	62.9 kB	--	19.9 kB
Slide	--	25.5 kB	--	8.71 kB
Slider	--	76.7 kB	--	24.3 kB
Snackbar	--	75.4 kB	--	23.6 kB
SnackbarContent	--	63.5 kB	--	20 kB
SpeedDial	--	86.3 kB	--	27.2 kB
SpeedDialAction	--	118 kB	--	37.5 kB
SpeedDialIcon	--	64.6 kB	--	20.3 kB
Step	--	62.6 kB	--	19.7 kB
StepButton	--	82.4 kB	--	26.1 kB
StepConnector	--	62.7 kB	--	19.8 kB
StepContent	--	69.2 kB	--	21.7 kB
StepIcon	--	64.6 kB	--	20.2 kB
StepLabel	--	68.6 kB	--	21.6 kB
Stepper	--	64.9 kB	--	20.5 kB
styles/createMuiTheme	--	16.5 kB	--	5.81 kB
SvgIcon	--	63 kB	--	19.7 kB
SwipeableDrawer	--	92.3 kB	--	28.9 kB
Tab	--	76.4 kB	--	24.2 kB
Table	--	62.5 kB	--	19.7 kB
TableBody	--	62.1 kB	--	19.5 kB
TableCell	--	64 kB	--	20.2 kB
TableContainer	--	61.9 kB	--	19.4 kB
TableFooter	--	62.1 kB	--	19.5 kB
TableHead	--	62.1 kB	--	19.5 kB
TablePagination	--	143 kB	--	41.9 kB
TableRow	--	62.5 kB	--	19.6 kB
TableSortLabel	--	77.5 kB	--	24.4 kB
Tabs	--	85.6 kB	--	27.2 kB
TextareaAutosize	--	5.12 kB	--	2.14 kB
TextField	--	125 kB	--	36.6 kB
ToggleButton	--	76.2 kB	--	24.1 kB
ToggleButtonGroup	--	63.2 kB	--	19.9 kB
Toolbar	--	62.3 kB	--	19.6 kB
Tooltip	--	102 kB	--	32.3 kB
TreeItem	--	74 kB	--	23.4 kB
TreeView	--	66.7 kB	--	21 kB
Typography	--	63.6 kB	--	19.9 kB
useAutocomplete	--	14.6 kB	--	5.26 kB
useMediaQuery	--	2.51 kB	--	1.06 kB
Zoom	--	23.5 kB	--	8.1 kB

Generated by 🚫 dangerJS against b3cd783

eps1lon

Thanks for working on this. Could you add a test illustrating the underlying issue?

rostislavbobo · 2020-01-20T18:07:19Z

@mbrookes, do you want me to revert all the default props that were set with a purpose to satisfy Framer X or we're good with these three?

rostislavbobo · 2020-01-20T18:46:09Z

I think that what @eps1lon meant is that we miss a test that would fail without the fix.

@eps1lon's question is clear (I reacted with a thumbs up), I'll add the test

My question to @mbrookes is about extra default props he added while adding Framer X. Since we can configure default prop outside of the code we ship to the users, we might wanna rollback that changes and add default prop values to framerConfig.js (as I showed it with Checkbox, Radio, and Switch)

mbrookes · 2020-01-20T21:01:14Z

@rostislavbobo Thanks for working on this, and for taking the time to wrap your head around the Framer code, which can be a bit convoluted. Good to see the automatic code generation paying dividends for newly introduced props. For a while it seemed more trouble than it was worth. 😅

To answer your question - unless there are other components that have an implicit dependency on a prop being undefined in order to determine its behaviour (is that a code smell?), then I think we are good with the other defaults.

eps1lon · 2020-01-20T22:34:25Z

unless there are other components that have in implicit dependency on a prop being undefined in order to determine its behaviour (is that a code smell?)

I think the actual code smell is computing some variable from context and props. Just like a component instance should either read from state or props (but not switch between) we shouldn't listen to context and props. It's a general issue with our form components where <FormControl disabled><Input disabled={false} /></FormControl> is allowed for some reason. The disabled state should live at a single point.

But we have to live with this for now. In v5 we should revisit this and decide if something lives in props or context.

… behavior

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

packages/material-ui/src/internal/SwitchBase.test.js

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

packages/material-ui/src/internal/SwitchBase.test.js

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

eps1lon · 2020-01-23T13:50:04Z

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

  describeConformance(<Checkbox checked />, () => ({
    classes,
    inheritComponent: IconButton,
    mount,
    refInstanceof: window.HTMLSpanElement,
    skip: ['componentProp'],
+    after: () => mount.cleanUp(),


Yeah that is a bit obscure if you're unfamiliar with the testing setup. Glad that you figured this out for yourself 👍

eps1lon · 2020-01-23T13:52:04Z

@rostislavbobo Great first pull request 👍. Happy to assist in future work.

rostislavbobo added 4 commits January 20, 2020 17:41

Update Framer X components

294f276

[Checkbox][Radio][Switch] Reset disabled prop to undefined

e06b2eb

[Checkbox][Radio][Switch] Update docs by resetting default disabled prop

5c3e6fc

Update framerConfig by specifying disabled prop for [Checkbox][Radio]…

d7d3165

…[Switch]

rostislavbobo requested a review from mbrookes January 20, 2020 16:27

oliviertassinari added the bug 🐛 Something doesn't work label Jan 20, 2020

eps1lon requested changes Jan 20, 2020

View reviewed changes

eps1lon added the PR: needs test The pull request can't be merged label Jan 20, 2020

This comment has been minimized.

Sign in to view

rostislavbobo added 4 commits January 21, 2020 12:31

[SwitchBase] Fix override disabled by prop test to check the expected…

860f0ae

… behavior

[Checkbox] Test disabled state with FormControl

3b176f0

[Radio] Test disabled state with FormControl

8652b8e

[Switch] Test disabled state with FormControl

4580023

rostislavbobo force-pushed the switch-base-ignores-form-control-disabled-state branch from eeeb594 to 4580023 Compare January 21, 2020 11:00

rostislavbobo requested a review from eps1lon January 21, 2020 11:20

mbrookes approved these changes Jan 21, 2020

View reviewed changes

oliviertassinari reviewed Jan 21, 2020

View reviewed changes

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

eps1lon approved these changes Jan 21, 2020

View reviewed changes

packages/material-ui/src/internal/SwitchBase.test.js Show resolved Hide resolved

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

eps1lon removed the PR: needs test The pull request can't be merged label Jan 21, 2020

rostislavbobo added 2 commits January 22, 2020 10:32

[SwitchBase] Test disabled against IconButton and input

2c52b24

[Checkbox][Radio][Switch] Test disabled attribute instead of class name

5c97816

eps1lon reviewed Jan 22, 2020

View reviewed changes

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

rostislavbobo commented Jan 22, 2020

View reviewed changes

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

rostislavbobo added 2 commits January 22, 2020 15:00

[SwitchBase] Update test by generalizing underlying component

85ea482

[Checkbox][Radio] Fix describeConformance by adding after callback

b3cd783

eps1lon approved these changes Jan 23, 2020

View reviewed changes

eps1lon changed the title ~~[SwitchBase] Ignores FormControl disabled state~~ [SwitchBase] Fix ignoring disabled from FormControl Jan 23, 2020

eps1lon merged commit 1092c22 into mui:master Jan 23, 2020

rostislavbobo deleted the switch-base-ignores-form-control-disabled-state branch January 23, 2020 14:31

EsoterikStare pushed a commit to EsoterikStare/material-ui that referenced this pull request Jan 23, 2020

[SwitchBase] Fix ignoring disabled from FormControl (mui#19319)

cd57bc1

This was referenced Feb 25, 2020

[Snyk] Upgrade @material-ui/core from 4.1.1 to 4.9.1 baaahs/sparklemotion#199

Closed

[Snyk] Upgrade @material-ui/core from 4.9.0 to 4.9.1 cybersecmoo/brownie-tub#19

Merged

[Snyk] Upgrade @material-ui/icons from 4.5.1 to 4.9.1 cybersecmoo/brownie-tub#20

Merged

oliviertassinari mentioned this pull request Mar 6, 2020

[RFC] Material-UI v5 🚀 #20012

Closed

snyk-bot mentioned this pull request Mar 22, 2020

[Snyk] Upgrade @material-ui/core from 4.9.1 to 4.9.4 mdfaizan7/react-bitConnect-client#1

Open

This was referenced Apr 16, 2020

[Snyk] Upgrade @material-ui/core from 4.3.3 to 4.9.7 reactioncommerce/reaction#6208

Merged

[Snyk] Upgrade: @material-ui/core, @material-ui/icons BrainBackup/bb8#100

Closed

mbrookes mentioned this pull request Jan 16, 2021

[core] Decide if a value belongs in props or context #24450

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SwitchBase] Fix ignoring disabled from FormControl #19319

[SwitchBase] Fix ignoring disabled from FormControl #19319

rostislavbobo commented Jan 20, 2020

mui-pr-bot commented Jan 20, 2020 •

edited

Loading

eps1lon left a comment

rostislavbobo commented Jan 20, 2020

This comment has been minimized.

rostislavbobo commented Jan 20, 2020

mbrookes commented Jan 20, 2020 •

edited

Loading

eps1lon commented Jan 20, 2020

eps1lon Jan 23, 2020

eps1lon commented Jan 23, 2020

[SwitchBase] Fix ignoring disabled from FormControl #19319

[SwitchBase] Fix ignoring disabled from FormControl #19319

Conversation

rostislavbobo commented Jan 20, 2020

mui-pr-bot commented Jan 20, 2020 • edited Loading

eps1lon left a comment

Choose a reason for hiding this comment

rostislavbobo commented Jan 20, 2020

This comment has been minimized.

rostislavbobo commented Jan 20, 2020

mbrookes commented Jan 20, 2020 • edited Loading

eps1lon commented Jan 20, 2020

eps1lon Jan 23, 2020

Choose a reason for hiding this comment

eps1lon commented Jan 23, 2020

mui-pr-bot commented Jan 20, 2020 •

edited

Loading

mbrookes commented Jan 20, 2020 •

edited

Loading