Skip to content

refactor(tooltip): use transition-delay instead of setTimeout#1699

Draft
spike-rabbit wants to merge 1 commit intomainfrom
refactor/tooltip/use-transition-delay
Draft

refactor(tooltip): use transition-delay instead of setTimeout#1699
spike-rabbit wants to merge 1 commit intomainfrom
refactor/tooltip/use-transition-delay

Conversation

@spike-rabbit
Copy link
Member

@spike-rabbit spike-rabbit commented Mar 20, 2026

Simplifies the tooltip by using a transition-delay instead of setTimeout to 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:

Code Coverage

Simplifies the tooltip by using a `transition-delay` instead of
`setTimeout` to delay displaying a toast.
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +73 to 87
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);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

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:

  1. Re-add the showTimeout property and clearShowTimeout method.

    private showTimeout?: ReturnType<typeof setTimeout>;
    
    private clearShowTimeout(): void {
      if (this.showTimeout) {
        clearTimeout(this.showTimeout);
        this.showTimeout = undefined;
      }
    }
  2. Call clearShowTimeout() in ngOnDestroy() and hide().

    ngOnDestroy(): void {
      this.clearShowTimeout();
      this.tooltipRef?.destroy();
    }
    
    protected hide(): void {
      this.clearShowTimeout();
      this.tooltipRef?.hide();
    }
  3. Update showTooltip to use setTimeout.

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant