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

[Select] Fix incorrect event.target type in onChange #15272

Merged
merged 5 commits into from
Apr 11, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions docs/src/pages/demos/dialogs/MaxWidthDialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ class MaxWidthDialog extends React.Component {
};

handleMaxWidthChange = event => {
this.setState({ maxWidth: event.target.value });
const maxWidth = event.target.value === 'false' ? false : event.target.value;

this.setState({ maxWidth });
};

handleFullWidthChange = event => {
Expand Down Expand Up @@ -84,7 +86,7 @@ class MaxWidthDialog extends React.Component {
id: 'max-width',
}}
>
<MenuItem value={false}>false</MenuItem>
<MenuItem value="false">false</MenuItem>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why changing the type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This had to be changed because MenuItem only accepts string | number | string[] | undefined for its type. The change in the handleMaxWidthChange covers converting this back to a boolean when this value is chosen!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could MenuItem accept boolean too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be addressed in #15245.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😩 We're breaking quite a bit of DOM/type safety for some tiny conveniences. We could just use an empty string and check for that. Otherwise we need to override the actual DOM types.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's passed straight down to the DOM node, yes. It's probably better to override this with unknown since it is also used as a child of Select. But it's also used without the Select context. In different contexts you might be missing the type checking. I think we should use either some type casting or add the runtime overhead that ensures a sound value type. The value of MenuItem should not be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should we keep the value of MenuItem as false and cast it to unknown?

Also, will there be issues trying to read event.target.value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event.target.value

They always end up as strings in the the DOM. With the exception of Radio and RadioGroup no component reads this value though. We try to only use the value from props.

So should we keep the value of MenuItem as false and cast it to unknown?

I think the Select already passes the value as the second argument to onChange. That value is the value from the prop and not the DOM. You could use that instead and just force it into state via any/unknown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I see. So the idea is to mostly use the value returned as the second param in the onChange event? I will push a change!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it depends. If you want the value you passed as a prop you use the second param (type unknown). If you want the value in the DOM you use event.target.value (type string).

<MenuItem value="xs">xs</MenuItem>
<MenuItem value="sm">sm</MenuItem>
<MenuItem value="md">md</MenuItem>
Expand Down
135 changes: 135 additions & 0 deletions docs/src/pages/demos/dialogs/MaxWidthDialog.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
import React from 'react';
import PropTypes from 'prop-types';
import { createStyles, Theme, withStyles, WithStyles } from '@material-ui/core/styles';
import Button from '@material-ui/core/Button';
import Dialog, { DialogProps } from '@material-ui/core/Dialog';
import DialogActions from '@material-ui/core/DialogActions';
import DialogContent from '@material-ui/core/DialogContent';
import DialogContentText from '@material-ui/core/DialogContentText';
import DialogTitle from '@material-ui/core/DialogTitle';
import FormControl from '@material-ui/core/FormControl';
import FormControlLabel from '@material-ui/core/FormControlLabel';
import InputLabel from '@material-ui/core/InputLabel';
import MenuItem from '@material-ui/core/MenuItem';
import Select from '@material-ui/core/Select';
import Switch from '@material-ui/core/Switch';

const styles = (theme: Theme) =>
createStyles({
form: {
display: 'flex',
flexDirection: 'column',
margin: 'auto',
width: 'fit-content',
},
formControl: {
marginTop: theme.spacing(2),
minWidth: 120,
},
formControlLabel: {
marginTop: theme.spacing(1),
},
});

export type MaxWidthDialogProps = WithStyles<typeof styles>;

export interface MaxWidthDialogState {
open: boolean;
fullWidth: boolean;
maxWidth: DialogProps['maxWidth'];
}

class MaxWidthDialog extends React.Component<MaxWidthDialogProps, MaxWidthDialogState> {
state: MaxWidthDialogState = {
open: false,
fullWidth: true,
maxWidth: 'sm',
};

handleClickOpen = () => {
this.setState({ open: true });
};

handleClose = () => {
this.setState({ open: false });
};

handleMaxWidthChange = (event: React.ChangeEvent<HTMLSelectElement>) => {
const maxWidth =
event.target.value === 'false' ? false : (event.target.value as DialogProps['maxWidth']);

this.setState({ maxWidth });
};

handleFullWidthChange = (event: React.ChangeEvent<HTMLInputElement>) => {
this.setState({ fullWidth: event.target.checked });
};

render() {
const { classes } = this.props;

return (
<React.Fragment>
<Button variant="outlined" color="primary" onClick={this.handleClickOpen}>
Open max-width dialog
</Button>
<Dialog
fullWidth={this.state.fullWidth}
maxWidth={this.state.maxWidth}
open={this.state.open}
onClose={this.handleClose}
aria-labelledby="max-width-dialog-title"
>
<DialogTitle id="max-width-dialog-title">Optional sizes</DialogTitle>
<DialogContent>
<DialogContentText>
You can set my maximum width and whether to adapt or not.
</DialogContentText>
<form className={classes.form} noValidate>
<FormControl className={classes.formControl}>
<InputLabel htmlFor="max-width">maxWidth</InputLabel>
<Select
value={this.state.maxWidth}
onChange={this.handleMaxWidthChange}
inputProps={{
name: 'max-width',
id: 'max-width',
}}
>
<MenuItem value="false">false</MenuItem>
<MenuItem value="xs">xs</MenuItem>
<MenuItem value="sm">sm</MenuItem>
<MenuItem value="md">md</MenuItem>
<MenuItem value="lg">lg</MenuItem>
<MenuItem value="xl">xl</MenuItem>
</Select>
</FormControl>
<FormControlLabel
className={classes.formControlLabel}
control={
<Switch
checked={this.state.fullWidth}
onChange={this.handleFullWidthChange}
value="fullWidth"
/>
}
label="Full width"
/>
</form>
</DialogContent>
<DialogActions>
<Button onClick={this.handleClose} color="primary">
Close
</Button>
</DialogActions>
</Dialog>
</React.Fragment>
);
}
}

(MaxWidthDialog as React.ComponentClass<MaxWidthDialogProps, MaxWidthDialogState>).propTypes = {
classes: PropTypes.object.isRequired,
} as any;

export default withStyles(styles)(MaxWidthDialog);