refactor(tooltip): use transition-delay instead of setTimeout#1699
refactor(tooltip): use transition-delay instead of setTimeout#1699spike-rabbit wants to merge 1 commit intomainfrom
transition-delay instead of setTimeout#1699Conversation
Simplifies the tooltip by using a `transition-delay` instead of `setTimeout` to delay displaying a toast.
There was a problem hiding this comment.
Code Review
This pull request refactors the tooltip implementation to use CSS transition-delay instead of setTimeout for delaying the tooltip's appearance. This simplifies the TypeScript code. However, this change introduces a performance regression by creating the tooltip component instance immediately on hover, even for brief mouse-overs. My review includes a detailed comment suggesting to revert to using setTimeout to debounce component creation, which is more performant, while retaining some of the readability improvements from this PR.
| private showTooltip(trigger: 'hover' | 'focus'): void { | ||
| const siTooltip = this.siTooltip(); | ||
| if (this.isDisabled() || !siTooltip) { | ||
| return; | ||
| } | ||
|
|
||
| this.clearShowTimeout(); | ||
|
|
||
| const delay = immediate ? 0 : 500; | ||
|
|
||
| this.showTimeout = setTimeout(() => { | ||
| this.tooltipRef ??= this.tooltipService.createTooltip({ | ||
| describedBy: this.describedBy, | ||
| element: this.elementRef, | ||
| placement: this.placement(), | ||
| tooltip: this.siTooltip, | ||
| tooltipContext: this.tooltipContext | ||
| }); | ||
| this.tooltipRef.show(); | ||
| }, delay); | ||
| this.tooltipRef ??= this.tooltipService.createTooltip({ | ||
| describedBy: this.describedBy, | ||
| element: this.elementRef, | ||
| placement: this.placement(), | ||
| tooltip: this.siTooltip, | ||
| tooltipContext: this.tooltipContext | ||
| }); | ||
| this.tooltipRef.show(trigger); | ||
| } |
There was a problem hiding this comment.
This refactoring simplifies the code by replacing setTimeout with a CSS transition-delay. However, as you noted in the pull request description, this introduces a performance regression.
With this change, the tooltip component is created and added to the DOM immediately on mouseenter, even if the user just quickly moves the mouse over the element without intending to see the tooltip. The previous implementation with setTimeout acted as a debounce, creating the tooltip component only after a 500ms delay. This is more performant, especially on pages with many tooltips, as it avoids creating components unnecessarily.
I recommend reintroducing the setTimeout to delay the creation of the tooltip component. This would also mean that the CSS-based delay and passing the trigger type to the component are no longer necessary, simplifying other parts of the code that were changed in this PR.
Here's how you could re-implement the debouncing logic while keeping the more descriptive trigger parameter:
-
Re-add the
showTimeoutproperty andclearShowTimeoutmethod.private showTimeout?: ReturnType<typeof setTimeout>; private clearShowTimeout(): void { if (this.showTimeout) { clearTimeout(this.showTimeout); this.showTimeout = undefined; } }
-
Call
clearShowTimeout()inngOnDestroy()andhide().ngOnDestroy(): void { this.clearShowTimeout(); this.tooltipRef?.destroy(); } protected hide(): void { this.clearShowTimeout(); this.tooltipRef?.hide(); }
-
Update
showTooltipto usesetTimeout.private showTooltip(trigger: 'hover' | 'focus'): void { const siTooltip = this.siTooltip(); if (this.isDisabled() || !siTooltip) { return; } this.clearShowTimeout(); const delay = trigger === 'focus' ? 0 : 500; this.showTimeout = setTimeout(() => { this.tooltipRef ??= this.tooltipService.createTooltip({ describedBy: this.describedBy, element: this.elementRef, placement: this.placement(), tooltip: this.siTooltip, tooltipContext: this.tooltipContext }); this.tooltipRef.show(); }, delay); }
This approach combines the performance benefits of the original implementation with the improved readability of using a trigger parameter.
Simplifies the tooltip by using a
transition-delayinstead ofsetTimeoutto delay displaying a toast.@dr-itz what is your opinion here?
Makes the code easier, but now always creates a tooltip instantly when hovering.
I guess this has a negative impact on performance.
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: