-
Notifications
You must be signed in to change notification settings - Fork 264
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
feat(ui5-popover): add "closeOnScroll" property #7526
Conversation
f718860
to
a4b6650
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.
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.
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.
LGTM
We discussed and agreed that this logic better fits in Popover.ts
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.
The approach with giving a new option to close the popover on scroll does not really solve the issues.
This property will be accessible only for popovers which the app developers have created themselfs, it will not be in their control when it comes to select, date picker or other controls with popovers.
Even for popovers for which they can set the property - they need to first know if there is sticky /fixed header on top which might not be the case.
Closing popover on scroll will also give inconsistent user experience with OpenUI5, which is good to have.
One thing which needs to be improved is to use the IntersectionObserver to see if popover opener is intersecting on scroll. This however does not fix the problem with sticky or fixed headers, which is a browser limitation.
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.
In PopoverRegistry use the IntersectionObserver to close the popover when opener is not intersecting anymore
Fixes #6829
Fixes #7356