-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[core] feat(Portal): Add portalContainer
context option
#6260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! just need a few documentation fixes
return container; | ||
}, [props.className, context.portalClassName]); | ||
return newPortalElement; | ||
}, [className, context.portalClassName, legacyContext.blueprintPortalClassName, stopPropagationEvents]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks for fixing this dependency list
portalContainer
option to to PortalContext/PortalProvider
portalContainer
context option
Another docs suggestion: let's update the link to React context docs. This line:
Should instead point to: https://react.dev/learn/passing-data-deeply-with-context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @braeden 👍🏽
Fixes #6210
Checklist
Changes proposed in this pull request:
This changes forces all (blueprint) children that who render below a given
PortalProvider
(like<Dialog/>
,<Popover2/>
) to have a common element whereReact.createPortal
usages become direct children of.To be more precise here,
portalContainer
acts ONLY as a parent element of a newdiv
and that newdiv
becomes the target container in theReact.createPortal
syntax -- sochildren
are rendered into that element.We also memoize the
PortalProvider
value to avoid thrashing on every re-render.Reviewers should focus on:
addStopPropagationListeners
,removeStopPropagationListeners
(andmaybeAddClass
) work -- usingReact.useEffect
's cleanup function to avoid leaving, what I believe would have been, dangling listeners when the portal unmounted.hasMounted
state, since it was purely duplicative withportalElement != null
Portal.defaultProps
and replaces it with an internal?? document?.body
fallback.Screenshot