Conversation
skjnldsv
added
bug
| * Called when popover is shown after the show delay | ||
| */ | ||
| onOpen() { | ||
| onOpened() { |
There was a problem hiding this comment.
Only renamed for consistency with the onClosed method name
Signed-off-by: skjnldsv <[email protected]>
Signed-off-by: skjnldsv <[email protected]>
skjnldsv
force-pushed
the
fix/actions-events
branch
from
ed4b48d to
8c997ce
Compare
| await this.useFocusTrap() | ||
| this.addEscapeStopPropagation() | ||
|
|
||
| setTimeout(() => { |
There was a problem hiding this comment.
Instead of reading a CSS variable and adding a new task with timeout, can we use a native event here?
this.getPopoverContentElement().addEventListener('transitionend', () => {
this.$emit('after-show')
}, { once: true })There was a problem hiding this comment.
Love it yeah! Didn't realized transitionend was an option
| * | ||
| * This event is emitted after `update:open` was emitted and the opening transition finished. | ||
| */ | ||
| this.$emit('opened') |
There was a problem hiding this comment.
Fix for NcPopover, feat for NcActions
There was a problem hiding this comment.
but the fix in ncpopover is unrelated to the feature for NcActions.
So this is semantically a feature PR ;)
There was a problem hiding this comment.
Yeah, was missing 🤷
ShGKme
added
enhancement
|
/backport to next |
| on: { | ||
| show: this.openMenu, | ||
| 'apply-show': this.onOpen, | ||
| 'after-show': this.onOpened, |
There was a problem hiding this comment.
This will now run quite late.
after-show will already await the next tick and the focus trap, so the additional tick in onOpened will cause a 1 tick delay which seems to not be needed. Probably the nextTick in onOpened can be removed.
There was a problem hiding this comment.
I think it's best to stay consistent. I'd like to avoid to have three different timings to watch for.
Two are perfectly fine:
- we start closing
- we are sure everything is finished and closed/hidden
susnux
changed the title
opened event / [NcPopover] animation wait
susnux
left a comment
susnux
left a comment
There was a problem hiding this comment.
Not sure about the additional tick we now have, but in general makes sense
Was still testing this, let me push a followup if it works ⌛ |
ShGKme
changed the title
opened event / [NcPopover] animation waitopened and closed events to handle closing/opening end | fix(NcPopover): correctly wait for animation end in after-show/after-hide events
|
A bit renamed for change log generation |
|
@ShGKme tested
|
|
Doesn't matter, much cleaner anyway |
|
@skjnldsv On my test: So it's 13ms longer, than timeout, but the animation might also have started later, than we think in the timeout |
3 participants
NcAcions
Following #6065
The evets are
after-hideandafter-show, notapply-hideandapply-showhttps://github.com/nextcloud/nextcloud-vue/blob/ae77137aee5097dcbe8cef6c53eb1ce3bc81c7ee/src/components/NcPopover/NcPopover.vue#L230-L237
NcPopover
The vue-floating lib does not properly compute how long it needs to wait to know the popup have been shown/hidden.
I investigated and it's only using
requestAnimationFramewhich in that case isn't enough to ensure its completion.Because we have our own custom animation variables, we need to manually check for it ourselves.
Tested on Server with log entries to see the duration (we use
--animation-quickwhich is 100ms for us)