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: ContextMenu2Popover, show/hide imperative APIs #5933

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Feb 9, 2023

Fixes #5527

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

Add two new APIs to the popover2 package:

  • <ContextMenu2Popover> encapsulates some of the opinionated popover implementation of <ContextMenu2> into a declarative API that can be used by Blueprint users. It's lower-level than <ContextMenu2> since users have to hook up onContextMenu handlers and cancel the default context menu themselves with e.preventDefault(), but this does offer a good declarative alternative to the deprecated ContextMenu.show() imperative API.
  • showContextMenu() + hideContextMenu() imperative APIs which use global state. These are provided as a convenience to make it easy to migrate to Blueprint v5, and they come with warnings about their implementation that consumers must heed. In general they should be fine, though, since we haven't seen many issues with imperative usage of ContextMenu.show() thus far.

Reviewers should focus on:

Docs for the new APIs

Screenshot

image

@adidahiya
Copy link
Contributor Author

update docs links

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

add unit tests

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

Fixup test

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya
Copy link
Contributor Author

use react.memo

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@adidahiya adidahiya merged commit 5c5456a into develop Feb 10, 2023
@adidahiya adidahiya deleted the ad/context-menu2-imperative branch February 10, 2023 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: consider imperative API for ContextMenu2
1 participant