-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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] fix(ContextMenu): create PopoverOverlay to avoid using popper.js #6720
base: develop
Are you sure you want to change the base?
Conversation
[core] fix(ContextMenu): create PopoverOverlay to avoid using popper.jsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
fix core:test:typeCheckBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
minor code style fix: destructure transitionDurationBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
8f7da3e
to
5e83160
Compare
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.
note that we no longer use ContextMenuPopover in Blueprint but it is still exported in the public API to avoid a breaking change. we should probably deprecate that component.
edit: no, actually, we can migrate ContextMenuPopover to use PopoverOverlay and continue supporting it.
menuContent === undefined || targetOffset === undefined || !isOpen ? undefined : ( | ||
<PopoverOverlay | ||
containerRef={popoverElement} | ||
content={<div onContextMenu={cancelContextMenu}>{menuContent}</div>} |
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.
previously, this cancelContextMenu
functionality happened in ContextMenuPopover
fwd more props to fix bugsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
migrate ContextMenuPopover to PopoverOverlayBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
fix resize sensor coverageBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Confirmed that this fixes the linked bug in React 18 strict mode on this branch: #6702 you can see that demo-app is rendering in strict mode: https://github.com/palantir/blueprint/blob/ad/fix-context-menu-snapping/packages/demo-app/src/index.tsx#L33-L37 |
Have a look at #4642 (comment) for how to get rid of the "virtual target" rendered in a portal, and use a popper.js virtual element instead. |
Fixes #5503
Checklist
Changes proposed in this pull request:
props.style
@popperjs/core
would not position our context menu "virtual target" element correctly in React 18 strict mode. Perhaps it's violating the rules somehow. I bet their advice would be to migrate to Floating UI, which we're not going to do any time soon (see Migrate to Floating UI #5739). So I figure the best approach is to stop using popper.js -- we don't even need any of its features for our context menu implementation.rootBoundary
prop is now ignored, as it doesn't make sense to have this be anything other than"viewport"
Reviewers should focus on:
No regressions in any popover-related components + context menu
Screenshot