Remove findDOMNode usage from Modal and Popover#46123
Merged
Conversation
It was not used within the app anyway
ravicious
added
the
no-changelog
ravicious
force-pushed
the
r7s/refactor-modal
branch
3 times, most recently
from
782ef74 to
f40b724
Compare
The only situation where they'd be needed if we were handling some nodes between iframes, but we don't use <iframe> even once in our code. Calls to those functions can be replaced with document and window. https://stackoverflow.com/questions/9845043/when-node-ownerdocument-is-not-window-document https://stackoverflow.com/questions/9183555/whats-the-point-of-document-defaultview
Initially I wanted to just remove the container prop, but as I sat down to it, I realized that the whole Portal component is not necessary as we can just replace it with calling createPortal directly. The onRendered prop of Portal was never used, rendering the handleRendered function unnecessary. Similarly, handlePortalRef was set to the ref prop on Portal. This function sets mountNode, but after we replaced ownerDocument with document, it's mountNode is no longer being used.
dialogRef used to point at the child of StyledModal. To get rid of findDOMNode, we replaced it by taking children directly from the ref set on StyledModal.
ravicious
force-pushed
the
r7s/refactor-modal
branch
from
f40b724 to
840b31e
Compare
ravicious
changed the title
ravicious
force-pushed
the
r7s/refactor-modal
branch
from
840b31e to
96dfd85
Compare
bl-nero
reviewed
Sep 5, 2024
| * The `reason` parameter can optionally be used to control the response to `onClose`. | ||
| */ | ||
| onClose?: ( | ||
| event: React.UIEvent, |
Contributor
There was a problem hiding this comment.
Depending on whether it was due to a click or keypress, the handler gets either a React event, or a native event (look how the event handlers are attached).
bl-nero
reviewed
Sep 6, 2024
| modalCss?: StyleFunction<any>; | ||
| children?: React.ReactElement; | ||
| /** | ||
| * Properties applied to the [`Backdrop`](/api/backdrop/) element. |
Contributor
There was a problem hiding this comment.
This link is quite possibly some relic of the forked third-party library, can we remove it?
| * | ||
| * invisible: Boolean - allows backdrop to keep bg color of parent eg: popup menu | ||
| */ | ||
| BackdropProps?: object; |
Contributor
There was a problem hiding this comment.
Consider extracting { invisible: boolean } from StyledBackdrop props and using it here. Then we can remove the "invisible: Boolean" comment from here and apply it directly as a documentation to the invisible field on the backdrop props.
ryanclark
approved these changes
Sep 6, 2024
bl-nero
approved these changes
Sep 9, 2024
Member
Author
|
@bl-nero I have to backport this because it's causing problems with certain modals, like the one to delete a notification rule (https://localhost:9002/?path=/story/teleporte-accessrequests-notificationroutingrules--create-and-view-valid-rule, "View" next to a notification rule and then click on the trash icon in the top right). The code goes into an infinite loop when focusing on the dialog el. See the thread on Slack. |
This was referenced Sep 9, 2024
mvbrock
pushed a commit
that referenced
this pull request
Sep 10, 2024
* Remove disablePortal prop It was not used within the app anyway * Get rid of ownerDocument and ownerWindow The only situation where they'd be needed if we were handling some nodes between iframes, but we don't use <iframe> even once in our code. Calls to those functions can be replaced with document and window. https://stackoverflow.com/questions/9845043/when-node-ownerdocument-is-not-window-document https://stackoverflow.com/questions/9183555/whats-the-point-of-document-defaultview * Remove container prop for Modal, replace Portal with createPortal Initially I wanted to just remove the container prop, but as I sat down to it, I realized that the whole Portal component is not necessary as we can just replace it with calling createPortal directly. The onRendered prop of Portal was never used, rendering the handleRendered function unnecessary. Similarly, handlePortalRef was set to the ref prop on Portal. This function sets mountNode, but after we replaced ownerDocument with document, it's mountNode is no longer being used. * Refactor Modal to use TypeScript * Remove RootRef dialogRef used to point at the child of StyledModal. To get rid of findDOMNode, we replaced it by taking children directly from the ref set on StyledModal. * Remove findDOMNode from Popover * Fix types for mouse & keyboard events * Insert whitespace between prop type declarations * Remove link from BackdropProps comment * Extract type for BackdropProps * Add JSDoc for children prop
mvbrock
pushed a commit
that referenced
this pull request
Sep 10, 2024
* Remove disablePortal prop It was not used within the app anyway * Get rid of ownerDocument and ownerWindow The only situation where they'd be needed if we were handling some nodes between iframes, but we don't use <iframe> even once in our code. Calls to those functions can be replaced with document and window. https://stackoverflow.com/questions/9845043/when-node-ownerdocument-is-not-window-document https://stackoverflow.com/questions/9183555/whats-the-point-of-document-defaultview * Remove container prop for Modal, replace Portal with createPortal Initially I wanted to just remove the container prop, but as I sat down to it, I realized that the whole Portal component is not necessary as we can just replace it with calling createPortal directly. The onRendered prop of Portal was never used, rendering the handleRendered function unnecessary. Similarly, handlePortalRef was set to the ref prop on Portal. This function sets mountNode, but after we replaced ownerDocument with document, it's mountNode is no longer being used. * Refactor Modal to use TypeScript * Remove RootRef dialogRef used to point at the child of StyledModal. To get rid of findDOMNode, we replaced it by taking children directly from the ref set on StyledModal. * Remove findDOMNode from Popover * Fix types for mouse & keyboard events * Insert whitespace between prop type declarations * Remove link from BackdropProps comment * Extract type for BackdropProps * Add JSDoc for children prop
10 tasks
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
3 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.
e counterpart: https://github.com/gravitational/teleport.e/pull/4970
findDOMNodeis going to be deprecated in React 19. I was able to replace all uses of it inModalandPopoverby using regular React refs, sincefindDOMNodewas used there mostly to get a DOM node and then read or write some stuff on it.I removed some functionality from
Modal, e.g. the ability to pass a custom container (if you didn't want to put your modal directly indocument.body) because it was not used anywhere in the project and it let me simplify the code a little bit. I also removed some extra abstractions that I deemed not to be necessary. Everything is explained in individual commit messages.The work on removing
findDOMNodeis not done, there's still a couple of callsites that use it. However, @bl-nero wants to work on the Popover component, so I'm pushing this PR for review so that he can branch off this work.