Provide nodeRef to Transition to fix StrictMode warning#3865
Conversation
| // Attach Transition's nodeRef to outer most wrapper for node.reflow: https://github.com/reactjs/react-transition-group/blob/c89f807067b32eea6f68fd6c622190d88ced82e2/src/Transition.js#L231 | ||
| return ( | ||
| <> | ||
| <div ref={wrapperRef}> |
There was a problem hiding this comment.
We could instead attach the nodeRef to the Underlay, however it doesn't exist in the isNonModal case with popover. Adding a wrapping div should be safe because Underlay and the overlay (Popover, Tray, Modal) all use position: fixed or position: absolute
LFDanLu
changed the title
LFDanLu
force-pushed
the
strict-mode-fixes
branch
from
8bcf8fd to
77faeb3
Compare
|
@reidbarber the rebase got a bit hairy, seems to have blown away some of your commits. I think I reconstructed the changes you had made, but lemme know if anything was missing |
LFDanLu
changed the title
got rid of clone children in tooltip also since it would break if wrapper existed around the tooltip component
needed to provide an actual DOM ref to OpenTransition nodeRef
technically we could attach it to any DOM node in the Modal/Tray/Popover since we dont have a transition on enter, but for correctness with reflow/.scrollTop we want it as the wrapping element
For some reason, Ref doesnt break TS strict linter but RefObject does...
LFDanLu
force-pushed
the
strict-mode-fixes
branch
from
8ef28db to
b04f490
Compare
ktabors
left a comment
ktabors
left a comment
There was a problem hiding this comment.
Code looks good. Haven't tested it yet.
| maxHeight: null | ||
| }, state); | ||
|
|
||
| // Attach Transition's nodeRef to outer most wrapper for node.reflow: https://github.com/reactjs/react-transition-group/blob/c89f807067b32eea6f68fd6c622190d88ced82e2/src/Transition.js#L231 |
There was a problem hiding this comment.
Why did you add the comment here and not to Modal.tsx or Tray.txs?
There was a problem hiding this comment.
No particular reason, I can definitely add that to those components as well
| } | ||
|
|
||
| export function Underlay({isOpen, isTransparent}: UnderlayProps) { | ||
| function Underlay({isOpen, isTransparent}: UnderlayProps, ref: Ref<HTMLDivElement>) { |
There was a problem hiding this comment.
do we still need the ref here? i am not quite seeing where it coming from or being used
There was a problem hiding this comment.
Good catch, forgot to remove that
5 participants
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
Note: test off the branch locally for strictmode.
Turn on StrictMode via the checkbox in the storybook top toolbar and test stories that have Popovers/Modals/Trays/ActionBar/any type of overlay. There shouldn't be a strict mode console warning about findDOMNode. Note that there will be a different crash in FocusScope that is fixed by #3878
Running the strict mode tests will yield a mixed bag of passes/fails due to other strict mode bugs. #3878 fixes many of the FocusScope related issues
🧢 Your Project:
RSP