Re-attach tooltip element listeners on v-if and v-show#28
Conversation
|
@copilot try again with the knowledge that the composable is to be used by the user to bind his own tooltip component to the package. |
Switched to |
|
@copilot why not simply watch |
Switched to |
3afbc5c to
2227765
Compare
2227765 to
84b3450
Compare
There was a problem hiding this comment.
Pull request overview
Updates @vingy/vueltip’s useVueltip composable so tooltip-element hover listeners are re-attached when the tooltip element is toggled via v-if/v-show, addressing the case where the tooltip DOM node is destroyed and recreated.
Changes:
- Replaces
onMountedlistener setup with awatch(show, ..., { flush: 'post' })-based lifecycle to attach/remove listeners as visibility changes. - Adds new browser tests covering listener attachment/removal for both
v-ifandv-show. - Updates the demo tooltip to use
v-if, and adds a changeset for a patch release.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/vueltip/src/composables.ts | Moves tooltip-element listener lifecycle to a post-flush watcher keyed on show. |
| packages/vueltip/src/composables.browser.test.ts | Adds browser tests for listener attach/remove across v-if/v-show and unmount. |
| demo/src/Tooltip.vue | Switches demo tooltip rendering from v-show to v-if. |
| .changeset/open-poets-visit.md | Patch changeset describing the v-if hover-visibility fix. |
| if (!value) return | ||
| const el = tooltipElement.value | ||
| if (!el) return | ||
| initialParent = el.parentElement |
There was a problem hiding this comment.
initialParent is reassigned every time show becomes true. When handleDialogModals moves the tooltip element into a <dialog>, subsequent show cycles can end up capturing the dialog as the new initialParent, breaking the logic that restores the tooltip back to its original container. Consider only setting initialParent once (e.g. when it’s still undefined) or storing the original parent separately from any temporary modal parent.
| initialParent = el.parentElement | |
| if (!initialParent && el.parentElement) { | |
| initialParent = el.parentElement | |
| } |
| watch( | ||
| show, | ||
| (value, _, onCleanup) => { | ||
| if (!value) return | ||
| const el = tooltipElement.value | ||
| if (!el) return | ||
| initialParent = el.parentElement | ||
|
|
||
| const onEnter = () => | ||
| (hoveredElement.value = | ||
| debouncedHoveredElement.value), | ||
| ) | ||
| tooltipElement.value?.addEventListener( | ||
| 'mouseleave', | ||
| () => (hoveredElement.value = undefined), | ||
| ) | ||
| }) | ||
| debouncedHoveredElement.value) | ||
| const onLeave = () => | ||
| (hoveredElement.value = undefined) | ||
|
|
||
| el.addEventListener('mouseenter', onEnter) | ||
| el.addEventListener('mouseleave', onLeave) | ||
|
|
||
| onCleanup(() => { | ||
| el.removeEventListener('mouseenter', onEnter) | ||
| el.removeEventListener('mouseleave', onLeave) | ||
| }) | ||
| }, | ||
| { flush: 'post' }, | ||
| ) |
There was a problem hiding this comment.
The listener watch(show, ..., { flush: 'post' }) now sets up initialParent/DOM-dependent state after render, but the existing handleDialogModals watcher (below) still runs with the default flush timing. On the first show=true, the modal watcher can run before initialParent is initialized (and before refs are populated), so it will no-op and won’t re-parent into the dialog until show changes again. Consider aligning the modal watcher to flush: 'post' (or otherwise ensure it runs after initialParent is captured).
| el.removeEventListener('mouseleave', onLeave) | ||
| }) | ||
| }, | ||
| { flush: 'post' }, |
There was a problem hiding this comment.
This watcher isn’t immediate, so if show is already true when useVueltip is set up (e.g. tooltip component mounts while an element is already hovered and debouncedHoveredElement is already set), the listener attachment won’t run until show flips false→true again. Consider adding immediate: true (keeping flush: 'post') or switching to a watchEffect gated on show.value + tooltipElement.value.
| { flush: 'post' }, | |
| { flush: 'post', immediate: true }, |
| const el = container.firstElementChild as HTMLElement | ||
| const removedTypes: string[] = [] | ||
| el.removeEventListener = (type: string) => { | ||
| removedTypes.push(type) | ||
| } | ||
|
|
There was a problem hiding this comment.
The tests that validate cleanup override el.removeEventListener and only record the event type. This can mask regressions where cleanup removes the wrong handler (or doesn’t pass the original function), since the assertions don’t validate the listener argument(s). Prefer using vi.spyOn(el, 'removeEventListener') and asserting it was called with both the expected event type and a function (and optionally verify the same function identity that was added).
| "@vingy/vueltip": patch | ||
| --- | ||
|
|
||
| When using v-if to control visibility of the tooltip element, hovering the tooltip, now keeps it visible. |
There was a problem hiding this comment.
Changeset text has an unnecessary comma that makes the sentence read awkwardly. Consider rephrasing (e.g. remove the comma) so it’s grammatically correct.
| When using v-if to control visibility of the tooltip element, hovering the tooltip, now keeps it visible. | |
| When using v-if to control visibility of the tooltip element, hovering the tooltip now keeps it visible. |
useVueltipattachedmouseenter/mouseleavelisteners ontooltipElementonly inonMounted, so toggling the tooltip element withv-ifwould destroy and re-create the element without ever re-attaching the listeners.Changes
composables.ts: ReplaceonMountedwithwatch(show, ..., { flush: 'post' }). Since the user binds the returnedshowcomputed tov-iforv-showon their tooltip element, watchingshowis the natural trigger — when it becomestrue,flush: 'post'guarantees the DOM has updated andtooltipElement.valueis populated before listeners are attached.onCleanupremoves the listeners before each re-run and on component unmount, preventing memory leaks. Bothv-if(element created/destroyed on show change) andv-show(element always in DOM) are supported.composables.browser.test.ts: New test file with sharedTooltipVIfandTooltipVShowcomponents defined at the file level, each a realistic tooltip implementation usinguseVueltipwithtooltipStylesandcontentbound — mirroring real user usage. Tests cover listener attach on show, listener removal on hide (v-if=false), and listener removal on component unmount.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.