refactor(ui5-popover): improve layouting, styling and positioning #779
Conversation
BREAKING CHANGE: stayOpenOnScroll is now removed - Popover will no longer close when the browser is scrolled and its parent (opener) is visible in the viewport.
| @@ -0,0 +1,27 @@ | |||
| import { isEscape } from "@ui5/webcomponents-base/dist/events/PseudoEvents.js"; | |||
|
|
|||
| let registry = []; | |||
There was a problem hiding this comment.
new Set() will be easier, you can add/delete instances directly (no need to filter)
There was a problem hiding this comment.
Set does not guarantee the order of pushed elements, therefore I decided to go for the list implementation
| return [...registry]; | ||
| }; | ||
|
|
||
| document.addEventListener("keydown", event => { |
There was a problem hiding this comment.
IMO make this a separate function, export it and run it explicitly from outside.
There was a problem hiding this comment.
Also, how are we sure this Esc is not related to some other element inside the popup?
There was a problem hiding this comment.
I am not :D Any proposals how we can move forward with that interaction?
packages/main/src/PopoverRegistry.js
Outdated
| @@ -0,0 +1,112 @@ | |||
| import { isClickInRect } from "./PopupUtils.js"; | |||
There was a problem hiding this comment.
Please move this file to a directory, try to keep the main only for components themselves
There was a problem hiding this comment.
I made a directory popup-utils name proposals are welcome
packages/main/src/PopoverRegistry.js
Outdated
| } | ||
|
|
||
| // remove top popovers from registry | ||
| Array(count).fill().forEach(() => { openedRegistry.pop(); }); |
- move utils to popup-utils folder - fix Vladi's initial comments
| addOpenedPopup(instance); | ||
| openedRegistry.push(instance); | ||
|
|
||
| attachScrollHandler(instance); |
There was a problem hiding this comment.
just to clarify why we need to attach scroll handler per instance + global scroll handler
There was a problem hiding this comment.
the scrooll event does not bubble outside the shadowroot => globoal handler will not be called. Therefore we need a scroll handler to each instance which should reposition all opened popovers on the page (this is because we need to handle the usa case of having nested scrollable popovers)
| const rect = domRef.getBoundingClientRect(); | ||
| let x, | ||
| y; | ||
| addOpenedPopover(this); |
There was a problem hiding this comment.
Two lines above there is reposition() called, and addOpenedPopover(this) itself will call reposition() (though runUpdateInterval). Is this intended?
There was a problem hiding this comment.
I wanted to call it synchronously so I can be sure that onbefore / after open will be called in the correct order.
MapTo0
changed the title
packages/main/test/sap/ui/webcomponents/main/pages/Popover2.html
Outdated
Show resolved
Hide resolved
The `stayOpenOnScroll` private property has been removed from the Popover long time ago with the PR #779
The `stayOpenOnScroll` private property has been removed from the Popover long time ago with the PR #779
FIXES: #776
and its parent (opener) is visible in the viewport.
BREAKING CHANGE: stayOpenOnScroll is now removed