-
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 combo-box #2497
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.
Could we also remove dedicated outsideClickListener
and instead update vaadin-combo-box-overlay
to override the corresponding method from vaadin-overlay?
_outsideClickListener(event) { |
d5b2f4c
to
e9047f7
Compare
0f9b794
to
9b3fc0d
Compare
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.
First batch of comments, will take a closer look later and test the actual behavior.
This is needed to avoid offset mistakes caused by opening animations applied to overlay.$.overlay
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 in desktop Chrome, Safari, Firefox, iOS Simulator 14.5 and Android Chrome.
Everything works fine, page scroll and virtual keyboard are handled properly 💯
Left a few non-blocking comments but overall LGTM 👍
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
Co-authored-by: Serhii Kulykov <iamkulykov@gmail.com>
Kudos, SonarCloud Quality Gate passed!
|
Kudos, SonarCloud Quality Gate passed!
|
This ticket/PR has been released with platform 22.0.0.alpha4 and is also targeting the upcoming stable 22.0.0 version. |
Removes a lot of the internal overlay positioning logic from
<vaadin-combo-box>
in favour of using thevaadin-overlay-position-mixin
.Fixes #1353