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] Remove unnecessary findDOMNode usage #14836

Merged
merged 1 commit into from
Mar 12, 2019
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Mar 10, 2019

Using some goodies from #13676 and #14536. docs/ is free of findDOMNode.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Mar 10, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Mar 10, 2019

No bundle size changes comparing a5b8011...67a300e

Generated by 🚫 dangerJS against 67a300e

@@ -31,10 +30,10 @@ function NativeSelects() {
name: 'hai',
});

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

@mbrookes mbrookes Mar 10, 2019

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

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

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

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

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))} />

@eps1lon eps1lon merged commit c638ef0 into mui:next Mar 12, 2019
@eps1lon eps1lon deleted the docs/findDOMNode branch March 12, 2019 07:53
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.

None yet

4 participants