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.

Sign up for GitHub

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

Jump to bottom

Fix PopupContextMenu in StatusBarPopover #912

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Fix PopupContextMenu in StatusBarPopover #912

merged 2 commits into from
Jul 15, 2024

Conversation

ignas-k
Copy link
Contributor

@ignas-k ignas-k commented Jul 15, 2024 •
edited
Loading

Changes

When PopupContextMenu is used as a nested popup in StatusBarPopover it is problematic because the logic of iTwinUI (Popover) and AppUI (Popup) clash since they portal to different places which then causes various issues (e.g. the nested Popup displays behind the main Popover or opening the nested Popup would close the Popover). As a simple fix I used portalTarget prop which will portal the Popup into the Popover (more specifically, the trigger button which is inside the Popover).

Additionally, PopupContextMenu will be deprecated in AppUI 4.16.0 and can be easily replaced with DropdownMenu or DropdownButton from iTwinUI.

Fixes #909.

Testing

Tested in standalone test-app.

@ignas-k ignas-k requested a review from a team as a code owner July 15, 2024 06:24
GerardasB
GerardasB approved these changes Jul 15, 2024
View reviewed changes
Copy link
Collaborator

@GerardasB GerardasB left a comment

Choose a reason for hiding this comment

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

The behavior will change if used w/o StatusBarPopover (i.e. StatusBar.Popup or any other Popup), did you verify that continues to work as well?
Do you plan to backport? If not - update the NextVersion.md

@ignas-k
Copy link
Contributor Author

ignas-k commented Jul 15, 2024

I tested this with other Popup components and even when it is not used in another popup. Everything seems to be working the same as before.

@ignas-k
Copy link
Contributor Author

ignas-k commented Jul 15, 2024

@Mergifyio backport release/4.15.x

@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 15, 2024 •
edited
Loading

backport release/4.15.x

✅ Backports have been created

@ignas-k ignas-k merged commit e467de1 into master Jul 15, 2024
19 checks passed
@ignas-k ignas-k deleted the fix-popup-context-menu branch July 15, 2024 11:18
mergify bot pushed a commit that referenced this pull request Jul 15, 2024
@ignas-k @mergify
Fix PopupContextMenu in StatusBarPopover (#912)
e9c7018 
* Fix PopupContextMenu issues

* Rush change

(cherry picked from commit e467de1)
@mergify mergify bot mentioned this pull request Jul 15, 2024
Merged
@ignas-k ignas-k mentioned this pull request Jul 15, 2024
Closed
GerardasB pushed a commit that referenced this pull request Jul 15, 2024
@mergify @ignas-k
Fix PopupContextMenu in StatusBarPopover (#912) (#914)
91a31de 
* Fix PopupContextMenu issues

* Rush change

(cherry picked from commit e467de1)

Co-authored-by: Ignas Kasinskas <138560995+ignas-k@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GerardasB GerardasB GerardasB approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression with opening PopupContextMenu from within Status bar popover
2 participants
@ignas-k @GerardasB