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.

Sign up for GitHub

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

[docs] Remove unnecessary findDOMNode usage #14836

Merged
merged 1 commit into from
Mar 12, 2019
+40 −40
Merged

[docs] Remove unnecessary findDOMNode usage #14836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 3 additions & 4 deletions docs/src/pages/demos/selects/NativeSelects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { makeStyles } from '@material-ui/core/styles';
import Input from '@material-ui/core/Input';
import OutlinedInput from '@material-ui/core/OutlinedInput';
Expand Down Expand Up @@ -31,10 +30,10 @@ function NativeSelects() {
name: 'hai',
});

const inputLabelRef = React.useRef(null);
Copy link
Member

@mbrookes mbrookes Mar 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

We should perhaps agree a pattern here. I find having Ref in the name adds clarity. when viewing the variable in isolation. Admittedly .current is a big giveaway, but when it's an actual component ref, I think it has merit. Similarly, having State in the name of state hooks...

Copy link
Member Author

Choose a reason for hiding this comment

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

So far I have seen *Ref used for ref objects and for return values from findDOMNode. I don't mind adding *Ref to return values from createRef. But it doesn't make much sense to do something like ref={ref => (containerRef = React.findDOMNode(ref))}.

I just think that it doesn't add much value if I append to every variable that gets some (create|use)Ref return value the *Ref suffix. At that point it only adds noise but no information.

Copy link
Member

@oliviertassinari oliviertassinari Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I have been suffixing all the ref variables with Ref. So no surprises here for me. I would vote for keeping it. But I don't have a strong opinion on the subject.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would vote for keeping it.

Keep what? *Ref for (create|use)Ref or *Ref for anything that is a reference to some object?

Copy link
Member

Choose a reason for hiding this comment

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

I would vote for the former...

Copy link
Member

@oliviertassinari oliviertassinari Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

*Ref for (create|use)Ref

@eps1lon Yes. My motivation is that it's an object we are mutating, it's not immutable. It's an important distinction.

Copy link
Member Author

@eps1lon eps1lon Mar 11, 2019 •
edited by oliviertassinari
Loading

Choose a reason for hiding this comment

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

My motivation is that it's an object we are mutating, it's not immutable. It's an important distinction.

That's not quite correct though. Every write to an instance field is mutating an object (this). I guess we can settle on "every var that can be handled by react via ref should have the Ref suffix"

// valid
<Component ref={ref => (this.someField = ref)} />
<Component ref={this.someRef} />
// invalid
<Component ref{ref => (this.notARef = ref)} />
<Component ref{ref => (this.containerRef = ReactDOM.findDOMNode(ref))} />

Copy link
Member

@oliviertassinari oliviertassinari Mar 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, if you feel it's better without the prefix we can remove it. I was optimizing for class components. In the hook realm, doing:

const containerRef = React.useRef();

allow us to make a distinction with the over variables. I'm seduced by this pattern. We can't confound a ref variable, that wraps a mutable variable, with an immutable variable that might comes, e.g. from a prop or useState.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well in that context it makes sense. I was mostly thinking about usage in this. For example

// containerRef is not mutable the same way as `React.createRef` would be mutable.
// The value can be changed but the reference not. Using the `Ref` suffix here is misleading
const { containerRef } = this;

<Component ref={ref => (this.containerRef = ReactDOM.findDOMNode(ref))} />

const inputLabel = React.useRef(null);
const [labelWidth, setLabelWidth] = React.useState(0);
React.useEffect(() => {
setLabelWidth(ReactDOM.findDOMNode(inputLabelRef.current).offsetWidth);
setLabelWidth(inputLabel.current.offsetWidth);
}, []);

const handleChange = name => event => {
Expand Down Expand Up @@ -189,7 +188,7 @@ function NativeSelects() {
<FormHelperText>Required</FormHelperText>
</FormControl>
<FormControl variant="outlined" className={classes.formControl}>
<InputLabel ref={inputLabelRef} htmlFor="outlined-age-native-simple">
<InputLabel ref={inputLabel} htmlFor="outlined-age-native-simple">
Age
</InputLabel>
<Select
Expand Down
7 changes: 3 additions & 4 deletions docs/src/pages/demos/selects/SimpleSelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import ReactDOM from 'react-dom';
import { makeStyles } from '@material-ui/core/styles';
import Input from '@material-ui/core/Input';
import OutlinedInput from '@material-ui/core/OutlinedInput';
Expand Down Expand Up @@ -31,10 +30,10 @@ function SimpleSelect() {
name: 'hai',
});

const inputLabelRef = React.useRef(null);
const inputLabel = React.useRef(null);
const [labelWidth, setLabelWidth] = React.useState(0);
React.useEffect(() => {
setLabelWidth(ReactDOM.findDOMNode(inputLabelRef.current).offsetWidth);
setLabelWidth(inputLabel.current.offsetWidth);
}, []);

function handleChange(event) {
Expand Down Expand Up @@ -223,7 +222,7 @@ function SimpleSelect() {
<FormHelperText>Required</FormHelperText>
</FormControl>
<FormControl variant="outlined" className={classes.formControl}>
<InputLabel ref={inputLabelRef} htmlFor="outlined-age-simple">
<InputLabel ref={inputLabel} htmlFor="outlined-age-simple">
Age
</InputLabel>
<Select
Expand Down
28 changes: 13 additions & 15 deletions docs/src/pages/demos/text-fields/ComposedTextField.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import { withStyles } from '@material-ui/core/styles';
import FilledInput from '@material-ui/core/FilledInput';
Expand All @@ -20,32 +19,36 @@ const styles = theme => ({
});

class ComposedTextField extends React.Component {
label = React.createRef();

state = {
labelWidth: 0,
name: 'Composed TextField',
};

componentDidMount() {
this.forceUpdate();
this.setState({ labelWidth: this.label.current.offsetWidth });
}

handleChange = event => {
this.setState({ name: event.target.value });
};

render() {
const { labelWidth, name } = this.state;
const { classes } = this.props;

return (
<div className={classes.container}>
<FormControl className={classes.formControl}>
<InputLabel htmlFor="component-simple">Name</InputLabel>
<Input id="component-simple" value={this.state.name} onChange={this.handleChange} />
<Input id="component-simple" value={name} onChange={this.handleChange} />
</FormControl>
<FormControl className={classes.formControl}>
<InputLabel htmlFor="component-helper">Name</InputLabel>
<Input
id="component-helper"
value={this.state.name}
value={name}
onChange={this.handleChange}
aria-describedby="component-helper-text"
/>
Expand All @@ -54,39 +57,34 @@ class ComposedTextField extends React.Component {
</FormControl>
<FormControl className={classes.formControl} disabled>
<InputLabel htmlFor="component-disabled">Name</InputLabel>
<Input id="component-disabled" value={this.state.name} onChange={this.handleChange} />
<Input id="component-disabled" value={name} onChange={this.handleChange} />
<FormHelperText>Disabled</FormHelperText>
</FormControl>
<FormControl className={classes.formControl} error>
<InputLabel htmlFor="component-error">Name</InputLabel>
<Input
id="component-error"
value={this.state.name}
value={name}
onChange={this.handleChange}
aria-describedby="component-error-text"
/>

<FormHelperText id="component-error-text">Error</FormHelperText>
</FormControl>
<FormControl className={classes.formControl} variant="outlined">
<InputLabel
ref={ref => {
this.labelRef = ReactDOM.findDOMNode(ref);
}}
htmlFor="component-outlined"
>
<InputLabel ref={this.label} htmlFor="component-outlined">
Name
</InputLabel>
<OutlinedInput
id="component-outlined"
value={this.state.name}
value={name}
onChange={this.handleChange}
labelWidth={this.labelRef ? this.labelRef.offsetWidth : 0}
labelWidth={labelWidth}
/>
</FormControl>
<FormControl className={classes.formControl} variant="filled">
<InputLabel htmlFor="component-filled">Name</InputLabel>
<FilledInput id="component-filled" value={this.state.name} onChange={this.handleChange} />
<FilledInput id="component-filled" value={name} onChange={this.handleChange} />
</FormControl>
</div>
);
Expand Down
29 changes: 13 additions & 16 deletions docs/src/pages/demos/text-fields/ComposedTextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import React, { ComponentClass } from 'react';
import ReactDOM from 'react-dom';
import PropTypes from 'prop-types';
import { createStyles, Theme, withStyles, WithStyles } from '@material-ui/core/styles';
import FilledInput from '@material-ui/core/FilledInput';
Expand All @@ -23,77 +22,75 @@ const styles = (theme: Theme) =>
export interface Props extends WithStyles<typeof styles> {}

interface State {
labelWidth: number;
name: string;
}

class ComposedTextField extends React.Component<Props, State> {
labelRef: HTMLElement | null | undefined;
label = React.createRef<HTMLElement>();

state = {
labelWidth: 0,
name: 'Composed TextField',
};

componentDidMount() {
this.forceUpdate();
this.setState({ labelWidth: this.label.current!.offsetWidth });
}

handleChange = (event: React.ChangeEvent<HTMLInputElement>) => {
this.setState({ name: event.target.value });
};

render() {
const { labelWidth, name } = this.state;
const { classes } = this.props;

return (
<div className={classes.container}>
<FormControl className={classes.formControl}>
<InputLabel htmlFor="component-simple">Name</InputLabel>
<Input id="component-simple" value={this.state.name} onChange={this.handleChange} />
<Input id="component-simple" value={name} onChange={this.handleChange} />
</FormControl>
<FormControl className={classes.formControl}>
<InputLabel htmlFor="component-helper">Name</InputLabel>
<Input
id="component-helper"
value={this.state.name}
value={name}
onChange={this.handleChange}
aria-describedby="component-helper-text"
/>
<FormHelperText id="component-helper-text">Some important helper text</FormHelperText>
</FormControl>
<FormControl className={classes.formControl} disabled>
<InputLabel htmlFor="component-disabled">Name</InputLabel>
<Input id="component-disabled" value={this.state.name} onChange={this.handleChange} />
<Input id="component-disabled" value={name} onChange={this.handleChange} />
<FormHelperText>Disabled</FormHelperText>
</FormControl>
<FormControl className={classes.formControl} error>
<InputLabel htmlFor="component-error">Name</InputLabel>
<Input
id="component-error"
value={this.state.name}
value={name}
onChange={this.handleChange}
aria-describedby="component-error-text"
/>
<FormHelperText id="component-error-text">Error</FormHelperText>
</FormControl>
<FormControl className={classes.formControl} variant="outlined">
<InputLabel
ref={ref => {
this.labelRef = ReactDOM.findDOMNode(ref!) as HTMLLabelElement | null;
}}
htmlFor="component-outlined"
>
<InputLabel ref={this.label} htmlFor="component-outlined">
Name
</InputLabel>
<OutlinedInput
id="component-outlined"
value={this.state.name}
value={name}
onChange={this.handleChange}
labelWidth={this.labelRef ? this.labelRef.offsetWidth : 0}
labelWidth={labelWidth}
/>
</FormControl>
<FormControl className={classes.formControl} variant="filled">
<InputLabel htmlFor="component-filled">Name</InputLabel>
<FilledInput id="component-filled" value={this.state.name} onChange={this.handleChange} />
<FilledInput id="component-filled" value={name} onChange={this.handleChange} />
</FormControl>
</div>
);
Expand Down
5 changes: 5 additions & 0 deletions packages/material-ui/src/FormLabel/FormLabel.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ export interface FormLabelProps extends StandardProps<FormLabelBaseProps, FormLa
error?: boolean;
filled?: boolean;
focused?: boolean;
/**
* Should only be used if ref forwarding `component` is used.
* This is imprecise if `<FormLabel component={SomeComponent} />` is used.
*/
ref?: React.Ref<HTMLElement>;
required?: boolean;
}

Expand Down
4 changes: 3 additions & 1 deletion packages/material-ui/src/InputLabel/InputLabel.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,6 @@ export type InputLabelClassKey =
| 'filled'
| 'outlined';

export default class InputLabel extends React.Component<InputLabelProps> {}
declare const InputLabel: React.ComponentType<InputLabelProps>;

export default InputLabel;