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] fix(ContextMenu2): detect dark theme correctly #4756

Merged
merged 5 commits into from
Jun 11, 2021

Conversation

adidahiya
Copy link
Contributor

@adidahiya adidahiya commented Jun 9, 2021

Fixes #4743

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Fix ContextMenu2 dark theme detection by restoring an inline <div> rendered as the popover target, so that we can attach a ref to it and properly query for dark theme. Before this change, we were checking for dark theme on the "virtual" target we rendered inside a <Portal>, which will almost always be an invalid check since applications rarely add .bp3-dark to the <body> element.
  • 🔥 BREAKING CHANGE: rename Classes.CONTEXT_MENU2_POPOVER2_TARGET to Classes.CONTEXT_MENU2_VIRTUAL_TARGET
  • 🔥 BREAKING CHANGE: add ref property back to ContextMenu2ChildrenProps after it was removed in [popover2] fix(ContextMenu2): simpler target positioning logic #4740 (sorry for the API whiplash). If upgrading from popover2 > 0.8.0 < 0.11.0 and you are using the advanced children render function API, you'll need to attach this ref to whatever you return from the children render function.

Reviewers should focus on:

N/A

Screenshot

2021-06-11 13 18 07

@blueprint-bot
Copy link

minor rename

Previews: documentation | landing | table

@adidahiya
Copy link
Contributor Author

This solution doesn't work because the popper ref needs to be attached to the root node returned from renderTarget, it can't be on a descendant "virtual element". Need to re-introduce the ref which was removed in #4740.

@blueprint-bot
Copy link

attach ref to container

Previews: documentation | landing | table

@blueprint-bot
Copy link

fix code comment

Previews: documentation | landing | table

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.

ContextMenu2 does not detect dark theme target correctly
2 participants