-
Notifications
You must be signed in to change notification settings - Fork 82
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
refactor: use overlay-position-mixin with context-menu and menu-bar #2917
Conversation
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.
Sorry, not a very productive review on this one. It's hard to figure out what's going on without knowing the component better. I can mostly ask for clarifications. Maybe someone with more experience should take a second look.
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.
Some comments on the code expanding the ideas by @sissbruecker. I still have yet to test context-menu and menu-bar manually, maybe then I will have more ideas.
Kudos, SonarCloud Quality Gate passed!
|
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.
Tested locally and everything works fine, and updated screenshots are actually correct 👍
This ticket/PR has been released with platform 22.0.0.beta2 and is also targeting the upcoming stable 22.0.0 version. |
This PR completes the planned overlay positioning refactor by removing internal logic from
<vaadin-context-menu>
and<vaadin-menu-bar>
in favour of using thevaadin-overlay-position-mixin
.The change not only cleans up the codebase but also makes (re)positioning of nested menus more fluent and stable:
Before:
menu-overlay-positioning-before.mp4
After:
menu-overlay-positioning-after.mp4
Related #2497, #2510 and #2567