[core] Remove RootRef usage#15347
Merged
Merged
Conversation
joshwooding
added
type: new feature
joshwooding
commented
Apr 14, 2019
Details of bundle changes.Comparing: 522c8f7...07ef2e9
|
joshwooding
force-pushed
the
remove-root-ref-usage
branch
from
April 14, 2019 16:34
9a8dac3 to
0375fb7
Compare
56 tasks
joshwooding
force-pushed
the
remove-root-ref-usage
branch
from
April 18, 2019 14:25
100cea9 to
883b0d2
Compare
joshwooding
force-pushed
the
remove-root-ref-usage
branch
from
April 18, 2019 14:56
883b0d2 to
a6571d3
Compare
joshwooding
force-pushed
the
remove-root-ref-usage
branch
from
April 18, 2019 15:12
7917766 to
07ef2e9
Compare
joshwooding
marked this pull request as ready for review
April 18, 2019 15:34
oliviertassinari
approved these changes
Apr 19, 2019
| const rootRef = React.useRef(); | ||
| // can be removed once we drop support for non ref forwarding class components | ||
| const handleOwnRef = React.useCallback(ref => { | ||
| rootRef.current = ReactDOM.findDOMNode(ref); |
Member
There was a problem hiding this comment.
I believe we can add the // StrictMode ready comment. If @eps1lon accepts it.
Member
There was a problem hiding this comment.
Yes. Actually every findDOMNode that get's an instance from a ref should be strict-mode.
| } | ||
|
|
||
| if (!rootRef.current.contains(doc.activeElement)) { | ||
| if (rootRef.current && !rootRef.current.contains(doc.activeElement)) { |
Member
There was a problem hiding this comment.
Should we warn if rootRef.current is not an instance of HTMLElement but the TrapFocus is enabled (like we do in the Popper's prop-types)? It's a broad question, it's no tight to this effort.
Member
There was a problem hiding this comment.
Sounds good. Probably better than checking for focus i.e. duck-typing.
eps1lon
approved these changes
Apr 20, 2019
| } | ||
|
|
||
| if (!rootRef.current.contains(doc.activeElement)) { | ||
| if (rootRef.current && !rootRef.current.contains(doc.activeElement)) { |
Member
There was a problem hiding this comment.
Sounds good. Probably better than checking for focus i.e. duck-typing.
| const rootRef = React.useRef(); | ||
| // can be removed once we drop support for non ref forwarding class components | ||
| const handleOwnRef = React.useCallback(ref => { | ||
| rootRef.current = ReactDOM.findDOMNode(ref); |
Member
There was a problem hiding this comment.
Yes. Actually every findDOMNode that get's an instance from a ref should be strict-mode.
| const lastFocus = React.useRef(); | ||
| const rootRef = React.useRef(); | ||
| // can be removed once we drop support for non ref forwarding class components | ||
| const handleOwnRef = React.useCallback(ref => { |
Member
There was a problem hiding this comment.
Nit: Rather use instance.
Suggested change
| const handleOwnRef = React.useCallback(ref => { | |
| const handleOwnRef = React.useCallback(instance => { |
A ref would be the the pointer-type i.e.
ref = { current: instance }
^^^^^^^^^^^^^^^^^^^^^ the ref as a pointer-like object
^^^^^^^^ the actual instance
ref = instance => {}
^^^^^^^^^^^^^^ the ref as a callback that is fired every time the instance changes
^^^^^^^^ the instance
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After #15291, is merged TrapFocus is the only usage of RootRef.
This would be a breaking change because TrapFocus can only accept components that forward refs which affects everything that uses a Modal.
Fade and Grow needed to forward refs as they're used inside TrapFocus. Maybe we should do the same for the other Transitions?