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] Document ref forwarding (requirements) #15298

Merged
merged 11 commits into from
Apr 14, 2019
Merged

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Apr 10, 2019

Close #14415


TODO:

eps1lon added 2 commits April 10, 2019 14:27
- include children (used in Slide and future Tooltip)
- be more specific in strict mode caveat
@eps1lon eps1lon added the docs Improvements or additions to the documentation label Apr 10, 2019
@eps1lon eps1lon added this to the v4 milestone Apr 10, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Apr 10, 2019

No bundle size changes comparing 5236588...64e2488

Generated by 🚫 dangerJS against 64e2488

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 10, 2019

Should we add a soft deprecation message for RootRef in the documentation? https://next.material-ui.com/api/root-ref/#rootref-api

@eps1lon eps1lon marked this pull request as ready for review April 11, 2019 15:20
@eps1lon eps1lon requested a review from mbrookes April 11, 2019 15:20
Copy link
Member

@mbrookes mbrookes left a comment

Choose a reason for hiding this comment

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

"?" in a review comment means I wasn't sure...

docs/src/pages/getting-started/faq/faq.md Outdated Show resolved Hide resolved
docs/src/pages/guides/api/api.md Outdated Show resolved Hide resolved
docs/src/pages/guides/api/api.md Outdated Show resolved Hide resolved
docs/src/pages/guides/api/api.md Outdated Show resolved Hide resolved
docs/src/pages/guides/composition/composition.md Outdated Show resolved Hide resolved
docs/src/pages/guides/composition/composition.md Outdated Show resolved Hide resolved
If you use class components for the cases described above you will still see
warnings in `React.StrictMode` and `React.unstable_ConcurrentMode`. We use
`ReactDOM.findDOMNode` internally for backwards compatibility. You can use `React.forwardRef`
and a designated prop in your class component to forward the `ref` to a built-in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
and a designated prop in your class component to forward the `ref` to a built-in
and a designated prop in your class component to forward the `ref` to a DOM element.

?

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'll be honest I don't know how to properly explain this. Maybe I'm using the term ref wrong maybe it isn't well defined.

If I refer to the term ref then I mean the return value from e.g. React.createRef(). So the ref is the full object { current: instance }. What we end up is attaching the ref object { current: ... } to the host component i.e. <div ref={{ current: ... }} />. Or in other words the ref is the thing you pass to the ref prop1 of a component. You'll probably still catch me doing <Component ref={ref => ...} /> where naming the parameter ref is probably a misnomer. <Component ref={current => ...} /> should be clearer.

The DOM element will then be ref.current. Or more generally ref.current will point to the component instance. For host components rendered with react-dom it will be an instance of a DOM Element.

Please tell me if that doesn't make sense. In the end the most important thing is that we don't confuse the reader. It's not that important to be technically accurate but that one understands how to fix the issue at hand.

1 ref gets a special treatment so calling it a ref might not be 100% accurate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
and a designated prop in your class component to forward the `ref` to a built-in
and a designated prop in your class component to forward the `ref` to a DOM

Copy link
Member

Choose a reason for hiding this comment

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

What I found confusing was the terminology "built-in component". What exactly does that refer to?

(I also overlooked the fact that a ref is a pointer to the full object, not just a discrete DOM element, so my suggested replacement wasn't any more helpful.)

Co-Authored-By: eps1lon <silbermann.sebastian@gmail.com>
buit-in, host -> DOM

Since we only support react-dom renderer the host is equivalent to the DOM.
@eps1lon eps1lon requested a review from mbrookes April 13, 2019 15:18
Co-Authored-By: eps1lon <silbermann.sebastian@gmail.com>
pages/api/click-away-listener.md Outdated Show resolved Hide resolved
pages/api/slide.md Outdated Show resolved Hide resolved
docs/src/modules/utils/generateMarkdown.js Outdated Show resolved Hide resolved
pages/api/button-base.md Outdated Show resolved Hide resolved
@eps1lon eps1lon merged commit 8ae684d into mui:next Apr 14, 2019
@eps1lon eps1lon deleted the docs/v4/refs branch April 14, 2019 10:08
@eps1lon eps1lon mentioned this pull request Apr 14, 2019
99 tasks
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.

[RFC] Refs in v4
4 participants