refactor(tooltip): move all logic into the service#1535
refactor(tooltip): move all logic into the service#1535spike-rabbit wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the tooltip logic by moving it from the directive into a dedicated service. This is a good architectural improvement that should make it easier to use tooltips dynamically.
I've found a few issues with the implementation:
- There's a critical issue regarding Server-Side Rendering (SSR) where browser-specific code is not properly guarded.
- The tests contain a focused test suite (
fdescribe) which must be removed. - There's an inconsistency in a delay value used in tests compared to the implementation.
- A minor simplification can be made in one of the directives.
Please see my detailed comments for suggestions on how to address these points.
projects/element-ng/form/si-form-validation-tooltip/si-form-validation-tooltip.spec.ts
Outdated
Show resolved
Hide resolved
| ) { | ||
| const nativeElement = this.element.nativeElement; | ||
|
|
||
| fromEvent(nativeElement, 'focus') | ||
| .pipe(takeUntil(this.destroy$)) | ||
| .subscribe(event => this.onFocus(event)); | ||
|
|
||
| fromEvent(nativeElement, 'mouseenter') | ||
| .pipe( | ||
| takeUntil(this.destroy$), | ||
| delayWhen(() => timer(500).pipe(takeUntil(fromEvent(nativeElement, 'mouseleave')))) | ||
| ) | ||
| .subscribe(() => this.onMouseEnter()); | ||
|
|
||
| private subscription?: Subscription; | ||
| fromEvent(nativeElement, 'focusout') | ||
| .pipe(takeUntil(this.destroy$)) | ||
| .subscribe(() => this.onFocusOut()); | ||
|
|
||
| fromEvent(nativeElement, 'mouseleave') | ||
| .pipe(takeUntil(this.destroy$)) | ||
| .subscribe(() => this.onMouseLeave()); | ||
| } |
There was a problem hiding this comment.
The TooltipRef constructor directly subscribes to DOM events using fromEvent. This will fail during Server-Side Rendering (SSR) as these events and nativeElement are browser-specific. This change removes a previous isPlatformBrowser check, introducing a regression.
This violates a general rule for our repository: 'When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).'
To fix this, you should ensure this logic only runs in a browser environment. The cleanest way would be to inject PLATFORM_ID in SiTooltipService and pass it down to TooltipRef.
Here is an example of how you could implement this:
-
Modify
SiTooltipServiceto injectPLATFORM_IDand pass it toTooltipRef:// projects/element-ng/tooltip/si-tooltip.service.ts @Injectable() export class SiTooltipService { private overlay = inject(Overlay); private platformId = inject(PLATFORM_ID); // Add this createTooltip(config: { /*...*/ }): TooltipRef { // ... return new TooltipRef( getOverlay(config.element, this.overlay, false, config.placement), config.element, injector, this.platformId // Pass it here ); } }
-
Update
TooltipRefto acceptplatformIdand guard the browser-specific code:// projects/element-ng/tooltip/si-tooltip.service.ts class TooltipRef { // ... constructor( private overlayRef: OverlayRef, private element: ElementRef, private injector: Injector | undefined, private platformId: object // Add this ) { if (isPlatformBrowser(this.platformId)) { const nativeElement = this.element.nativeElement; // Move all fromEvent subscriptions inside this block fromEvent(nativeElement, 'focus') .pipe(takeUntil(this.destroy$)) .subscribe(event => this.onFocus(event)); // ... other subscriptions } } // ... }
You will also need to add the necessary imports for PLATFORM_ID and isPlatformBrowser.
References
- When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like
isPlatformBrowserto prevent errors during Server-Side Rendering (SSR).
projects/element-ng/form/si-form-validation-tooltip/si-form-validation-tooltip.directive.ts
Outdated
Show resolved
Hide resolved
projects/element-ng/form/si-form-validation-tooltip/si-form-validation-tooltip.spec.ts
Outdated
Show resolved
Hide resolved
bebbc60 to
534dc16
Compare
We currently cannot attach tooltips dynamically on our components if the host element is interactive element. When having the tooltip as a service, we can do this. Alternatives considered: - extend the tooltip directive - make the tooltip input a model
534dc16 to
c72e8db
Compare
| @@ -35,20 +85,25 @@ class TooltipRef { | |||
| const tooltipRef: ComponentRef<TooltipComponent> = this.overlayRef.attach(toolTipPortal); | |||
|
|
|||
| const positionStrategy = getPositionStrategy(this.overlayRef); | |||


We currently cannot attach tooltips dynamically
on our components if the host element is interactive element.
When having the tooltip as a service, we can do this.
Alternatives considered: