-
Notifications
You must be signed in to change notification settings - Fork 95
feat(NcActions): add opened and closed events to handle closing/opening end | fix(NcPopover): correctly wait for animation end in after-show/after-hide events
#6683
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1179,12 +1179,15 @@ | |
| }, | ||
|
|
||
| emits: [ | ||
| 'click', | ||
| 'blur', | ||
| 'focus', | ||
|
|
||
| 'close', | ||
| 'closed', | ||
| 'open', | ||
| 'opened', | ||
| 'update:open', | ||
| 'close', | ||
| 'focus', | ||
| 'blur', | ||
| 'click', | ||
| ], | ||
|
|
||
| setup(props) { | ||
|
|
@@ -1471,10 +1474,17 @@ | |
| /** | ||
| * Called when popover is shown after the show delay | ||
| */ | ||
| onOpen() { | ||
| onOpened() { | ||
| this.$nextTick(() => { | ||
| this.focusFirstAction(null) | ||
| this.resizePopover() | ||
|
|
||
| /** | ||
| * Event emitted when the popover menu is opened. | ||
| * | ||
| * This event is emitted after `update:open` was emitted and the opening transition finished. | ||
| */ | ||
| this.$emit('opened') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Feature not fix (new event)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix for NcPopover, feat for NcActions
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but the fix in ncpopover is unrelated to the feature for NcActions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, was missing 🤷 |
||
| }) | ||
| }, | ||
|
|
||
|
|
@@ -1516,7 +1526,7 @@ | |
| return this.$refs.menu.querySelector('li.active') | ||
| }, | ||
| /** | ||
| * @return {NodeListOf<HTMLElement>} | ||
| */ | ||
| getFocusableMenuItemElements() { | ||
| return this.$refs.menu.querySelectorAll(focusableSelector) | ||
|
|
@@ -1901,9 +1911,9 @@ | |
| }, | ||
| on: { | ||
| show: this.openMenu, | ||
| 'apply-show': this.onOpen, | ||
| 'after-show': this.onOpened, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will now run quite late.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's best to stay consistent. I'd like to avoid to have three different timings to watch for.
|
||
| hide: this.closeMenu, | ||
| 'apply-hide': this.onClosed, | ||
| 'after-hide': this.onClosed, | ||
| }, | ||
| }, | ||
| [ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,7 +174,6 @@ import NcPopoverTriggerProvider from './NcPopoverTriggerProvider.vue' | |
| * @typedef {import('focus-trap').FocusTargetValueOrFalse} FocusTargetValueOrFalse | ||
| * @typedef {FocusTargetValueOrFalse|() => FocusTargetValueOrFalse} SetReturnFocus | ||
| */ | ||
|
|
||
| export default { | ||
| name: 'NcPopover', | ||
|
|
||
|
|
@@ -239,6 +238,7 @@ export default { | |
| data() { | ||
| return { | ||
| internalShown: this.shown, | ||
| animationDuration: 100, | ||
| } | ||
| }, | ||
|
|
||
|
|
@@ -254,6 +254,7 @@ export default { | |
|
|
||
| mounted() { | ||
| this.checkTriggerA11y() | ||
| this.animationDuration = parseInt(getComputedStyle(this.$el).getPropertyValue('--animation-quick')) || 100 | ||
| }, | ||
|
|
||
| beforeDestroy() { | ||
|
|
@@ -385,22 +386,27 @@ export default { | |
| await this.useFocusTrap() | ||
| this.addEscapeStopPropagation() | ||
|
|
||
| setTimeout(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 })
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Love it yeah! Didn't realized |
||
| /** | ||
| * Triggered after the tooltip was visually displayed. | ||
| * | ||
| * This is different from the 'show' and 'apply-show' which | ||
| * run earlier than this where there is no guarantee that the | ||
| * tooltip is already visible and in the DOM. | ||
| */ | ||
| this.$emit('after-show') | ||
| this.$emit('after-show') | ||
| }, this.animationDuration) | ||
| }, | ||
| afterHide() { | ||
| async afterHide() { | ||
skjnldsv marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| this.clearFocusTrap() | ||
| this.clearEscapeStopPropagation() | ||
| /** | ||
| * Triggered after the tooltip was visually hidden. | ||
| */ | ||
| this.$emit('after-hide') | ||
|
|
||
| setTimeout(() => { | ||
| /** | ||
| * Triggered after the tooltip was visually hidden. | ||
| */ | ||
| this.$emit('after-hide') | ||
| }, this.animationDuration) | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
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.
Only renamed for consistency with the
onClosedmethod name