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

[docs] Show how to combine OutlinedInput and FilledInput #12968

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Sep 23, 2018

Closes #12965.

@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Sep 23, 2018
@oliviertassinari oliviertassinari force-pushed the docs-filled-outlined-input branch from 2056040 to d1f19e4 Compare September 23, 2018 09:48
@oliviertassinari oliviertassinari merged commit d0ab350 into mui:master Sep 23, 2018
@oliviertassinari oliviertassinari deleted the docs-filled-outlined-input branch September 23, 2018 10:02
@@ -17,10 +20,16 @@ const styles = theme => ({
});

class ComposedTextField extends React.Component {
labelRef = null;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use the modern React.createRef API.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could but it more boilerplate (like in the TextField source).

<FormControl className={classes.formControl} variant="outlined">
<InputLabel
ref={ref => {
this.labelRef = ReactDOM.findDOMNode(ref);
Copy link
Member

Choose a reason for hiding this comment

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

I believe you don't need to do this anymore. The ref already grants access to the dom node.

Copy link
Member Author

Choose a reason for hiding this comment

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

How?

Copy link
Member

Choose a reason for hiding this comment

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

Well not in this case because InputLabel is not properly forwarding the ref. I assumed it did which was wrong. I always try to avoid findDOMNode since it's usage in refs is discourage by React.

If it would be forwarded we could reduce the boilerplate you wanted to avoid by using callback refs by simply using createRef and then passing that to InputLabel#ref.

facebook/react#4936 has additional explanation about findDOMNode(ref).

Considering how much customization we allow findDOMNode is probably safer and since it works there's no need for a change in my book. I just spotted something unusual (for me).

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I understand the warning about using findDOMNode, it's about educating people that React might provide another way to do what they want. Many people access the DOM when they don't need to. Hence the warning. In our case, it's fine.

marcelpanse pushed a commit to marcelpanse/material-ui that referenced this pull request Oct 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants