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

feat(ui5-popover): add "closeOnScroll" property #7526

Closed
wants to merge 4 commits into from

Conversation

TeodorTaushanov
Copy link
Member

@TeodorTaushanov TeodorTaushanov commented Aug 31, 2023

Fixes #6829
Fixes #7356

@TeodorTaushanov TeodorTaushanov changed the title feat(ui5-popover): add "autoCloseOnScroll" property feat(ui5-popover): add "closeOnScroll" property Sep 1, 2023
@TeodorTaushanov TeodorTaushanov requested review from ilhan007 and a team September 1, 2023 14:22
Copy link
Contributor

@georgimkv georgimkv left a comment

Choose a reason for hiding this comment

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

Edit the pull request description to have every issue that will be closed on a separate line each with the keyword "Fixes" otherwise Github will only automatically close the first issue:

image

packages/main/src/Popover.ts Show resolved Hide resolved
Copy link
Contributor

@alexandar-mitsev alexandar-mitsev left a comment

Choose a reason for hiding this comment

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

Fixes #6829 #7356 #7173

This change alone is not fixing any of the issues, so don't mark them with fixed. More instructions and code change is needed for the reporters.

Copy link
Contributor

@georgimkv georgimkv left a 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

Copy link
Contributor

@alexandar-mitsev alexandar-mitsev left a 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.

Copy link
Contributor

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

@nnaydenow nnaydenow deleted the popover_autoclose branch October 20, 2023 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants