Skip to content
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

[popover2] feat(ContextMenu2): support popoverProps.rootBoundary #5423

Merged
merged 8 commits into from
Aug 11, 2022

Conversation

GalvinGao
Copy link
Contributor

@GalvinGao GalvinGao commented Jul 9, 2022

Fixes #5411

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • To enable overriding the default rootBoundary value in ContextMenu2 component.

Reviewers should focus on:

  • Default value done right? (or not?)

Screenshot

N/A: I'm unsure exactly how to test such change. I've tried to just add a Example component in the docs by following what other documents appear to do, but I found that the problem mentioned in #5411 is not reproducible within the docs-app for unknown reasons. Therefore I'm unsure if adding the rootBoundary props to the ContextMenu2 will actually open the door for developers to mitigate this issue. Any help would be appreciated.

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @GalvinGao! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@adidahiya adidahiya changed the title [popover2] fix: wrong position when ContextMenu2 invoked from Drawer [popover2] feat(ContextMenu): allow overriding popover rootBoundary Jul 19, 2022
@adidahiya
Copy link
Contributor

@GalvinGao I edited your change to have a more minimal code diff.

I think we should test this change in the docs app or a unit test before merging. You could add a ContextMenu2 to the Drawer example in the docs-app for testing: https://github.com/palantir/blueprint/blob/develop/packages/docs-app/src/examples/core-examples/drawerExample.tsx

You may need to set boundary instead of rootBoundary to fix the problem. See the popper.js docs: https://popper.js.org/docs/v2/utils/detect-overflow/#boundary

@adidahiya
Copy link
Contributor

@GalvinGao are you still interested in this PR? Can you add a unit test?

@GalvinGao
Copy link
Contributor Author

@GalvinGao are you still interested in this PR? Can you add a unit test?

Just got time. Gonna add it today or tomorrow.

@GalvinGao
Copy link
Contributor Author

Added docs & unit tests. Ready for review ;)

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good :shipit:

image

@adidahiya adidahiya changed the title [popover2] feat(ContextMenu): allow overriding popover rootBoundary [popover2] feat(ContextMenu2): support popoverProps.rootBoundary Aug 11, 2022
@adidahiya adidahiya merged commit b3a14f1 into palantir:develop Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initial location of ContextMenu2 is misplaced when triggered inside a Drawer
4 participants