refactor(tooltip): align to new guidelines #1245
Open
+386
−173
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dauriamarco
added
the
breaking-changes
📝 WalkthroughWalkthroughRemoved the public Possibly related PRs
Suggested labelscore-team-discussion 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (10)
📒 Files selected for processing (9)
🧰 Additional context used🧠 Learnings (18)📓 Common learnings📚 Learning: 2025-12-16T16:19:17.950ZApplied to files:
📚 Learning: 2025-12-23T09:24:35.163ZApplied to files:
📚 Learning: 2025-12-23T09:24:57.843ZApplied to files:
📚 Learning: 2025-12-23T09:24:40.395ZApplied to files:
📚 Learning: 2025-12-11T10:09:01.564ZApplied to files:
📚 Learning: 2025-12-15T10:05:59.100ZApplied to files:
📚 Learning: 2025-12-17T04:34:55.597ZApplied to files:
📚 Learning: 2025-12-15T07:16:53.762ZApplied to files:
📚 Learning: 2025-12-08T11:25:51.584ZApplied to files:
📚 Learning: 2025-12-08T11:24:45.272ZApplied to files:
📚 Learning: 2025-12-15T07:17:06.981ZApplied to files:
📚 Learning: 2025-12-15T07:16:32.082ZApplied to files:
📚 Learning: 2025-12-05T08:00:38.407ZApplied to files:
📚 Learning: 2025-12-01T04:01:27.365ZApplied to files:
📚 Learning: 2025-12-04T05:50:17.637ZApplied to files:
📚 Learning: 2025-12-01T14:12:11.111ZApplied to files:
📚 Learning: 2025-12-30T13:52:33.581ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
projects/element-ng/tooltip/si-tooltip.directive.spec.ts (2)
115-124: Use tick(0) for focus events for consistency.Line 117 uses
tick(500)after a focus event, but focus shows tooltips immediately (0ms delay). Whiletick(500)works because it advances past the 0ms timeout, it's inconsistent with the explicit focus test at line 48 and misleading about the actual timing.🔎 Suggested fix for test clarity
it('should render the template', async () => { button.dispatchEvent(new Event('focus')); - jasmine.clock().tick(500); + jasmine.clock().tick(0); await fixture.whenStable(); expect(document.querySelector('.tooltip')?.innerHTML).toContain('Template content'); button.dispatchEvent(new Event('focusout')); - jasmine.clock().tick(500); await fixture.whenStable(); expect(document.querySelector('.tooltip')).toBeFalsy(); });Note: The
tick()afterfocusoutcan also be removed sincehide()doesn't use a delay.
126-138: Use tick(0) for focus events for consistency.Line 131 uses
tick(500)after a focus event, but should usetick(0)for the same reasons as the previous test.🔎 Suggested fix for test clarity
it('should render the template with context', async () => { fixture.componentInstance.tooltipContext = { tooltip: 'test' }; fixture.changeDetectorRef.markForCheck(); fixture.detectChanges(); button.dispatchEvent(new Event('focus')); - jasmine.clock().tick(500); + jasmine.clock().tick(0); await fixture.whenStable(); expect(document.querySelector('.tooltip')?.innerHTML).toContain('Template content test'); button.dispatchEvent(new Event('focusout')); - jasmine.clock().tick(500); await fixture.whenStable(); expect(document.querySelector('.tooltip')).toBeFalsy(); });Note: The
tick()afterfocusoutcan also be removed sincehide()doesn't use a delay.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (10)
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (11)
api-goldens/element-ng/tooltip/index.api.mdplaywright/e2e/element-examples/si-tooltip.spec.tsplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yamlprojects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.tssrc/app/examples/si-tooltip/si-tooltip.htmlsrc/app/examples/si-tooltip/si-tooltip.ts
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.272Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
📚 Learning: 2025-12-11T10:09:01.564Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts:83-104
Timestamp: 2025-12-11T10:09:01.564Z
Learning: In the auto-collapsable-list directive tests (projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts), jasmine.clock() successfully controls the directive's setTimeout-based layout logic in zoneless mode. Mock timers work fine here, unlike in select component overflow tests where real setTimeout waits are required.
Applied to files:
playwright/e2e/element-examples/si-tooltip.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
playwright/e2e/element-examples/si-tooltip.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
playwright/e2e/element-examples/si-tooltip.spec.tssrc/app/examples/si-tooltip/si-tooltip.tsprojects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.ts
📚 Learning: 2025-12-16T16:19:17.950Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1207
File: playwright/snapshots/si-filtered-search.spec.ts-snapshots/si-filtered-search--si-filtered-search-playground--data-entered.yaml:38-38
Timestamp: 2025-12-16T16:19:17.950Z
Learning: Do not review files in the playwright/snapshots directory, as they contain auto-generated test artifacts. Exclude any YAML snapshot files under playwright/snapshots from code reviews in the siemens/element repository. If a YAML file is not an auto-generated snapshot, verify its origin before skipping.
Applied to files:
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yaml
📚 Learning: 2025-12-11T14:44:11.278Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1185
File: projects/element-ng/navbar/si-navbar-item/si-navbar-item.component.html:11-11
Timestamp: 2025-12-11T14:44:11.278Z
Learning: The si-icon component in siemens/element-ng/icon automatically applies aria-hidden="true" to its content. Do not suggest adding aria-hidden="true" to si-icon elements, as accessibility is already handled internally by the component.
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-04T11:54:31.132Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1133
File: projects/element-ng/tour/si-tour.service.spec.ts:21-26
Timestamp: 2025-12-04T11:54:31.132Z
Learning: In the siemens/element repository, all components are standalone by default and do not require the explicit `standalone: true` flag. Components should be added to the `imports` array in TestBed configuration, not the `declarations` array.
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-09T14:33:54.441Z
Learnt from: Killusions
Repo: siemens/element PR: 998
File: src/app/examples/si-chat-messages/si-ai-message.ts:24-43
Timestamp: 2025-12-09T14:33:54.441Z
Learning: In src/app/examples/ (example/demo code) directories, allow and expect code duplication. Treat duplication as acceptable for demonstration code that aims to illustrate usage rather than enforce DRY. Do not enforce refactoring to remove duplication in example files; focus on readability and clarity of examples. This applies to all TypeScript files under src/app/examples (including nested subdirectories like si-chat-messages).
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-23T09:24:35.163Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/translate/index.api.md:12-84
Timestamp: 2025-12-23T09:24:35.163Z
Learning: In the siemens/element repository, do not review files under the api-goldens/ directory (e.g., api-goldens/**/index.api.md) since they are auto-generated by API Extractor. Exclude these from review checks and focus on source files that are not auto-generated.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-23T09:24:57.843Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/index.api.md:324-325
Timestamp: 2025-12-23T09:24:57.843Z
Learning: Do not review files in the api-goldens directory, as they are auto-generated API reports produced by API Extractor.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-23T09:24:40.395Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/module-federation/index.api.md:7-11
Timestamp: 2025-12-23T09:24:40.395Z
Learning: In the siemens/element repository, do not review any files under the api-goldens directory, as they are auto-generated API reports produced by API Extractor. These MD files should be excluded from review unless explicitly overridden.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-08T11:25:51.584Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.584Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-08T11:24:45.272Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.272Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
playwright/e2e/element-examples/si-tooltip.spec.ts (2)
10-10: LGTM! Auto placement coverage added.The addition of 'auto' to the directions array appropriately expands test coverage for the new automatic placement functionality.
14-14: LGTM! Focus-based activation aligns with accessibility guidelines.The switch from
dispatchEvent('mouseenter')tofocus()correctly reflects the new tooltip behavior where focus triggers immediate display, which improves accessibility and test reliability.projects/element-ng/tooltip/si-tooltip.directive.ts (8)
44-49: LGTM! showDelay input replaces triggers-based logic.The new
showDelayinput (default 500ms) provides a cleaner, more intuitive API for controlling tooltip display timing compared to the removedtriggersinput.
66-66: LGTM! Timeout management property added.The
showTimeoutproperty correctly stores the browser timeout ID for proper cleanup.
75-80: LGTM! Proper timeout cleanup.The
clearShowTimeoutmethod correctly clears pending timeouts and resets the reference, preventing memory leaks and unwanted delayed displays.
82-100: LGTM! Well-structured tooltip display logic.The
showTooltipmethod implements proper debouncing by:
- Clearing any pending timeout before scheduling a new one (line 88)
- Supporting immediate display (0ms) for focus vs delayed display for hover (line 90)
- Efficiently reusing tooltip refs with the
??=operator (line 93)The implementation prevents race conditions and ensures predictable tooltip behavior.
102-105: LGTM! Focus triggers immediate display.Passing
truetoshowTooltipensures focus events display tooltips immediately (0ms delay), which aligns with accessibility best practices for keyboard navigation.
107-110: LGTM! Hover respects configured delay.Passing
falsetoshowTooltipensures mouseenter events respect theshowDelayconfiguration (default 500ms), preventing accidental tooltip displays during rapid mouse movements.
112-117: LGTM! Proper timeout cancellation on hide.The
hidemethod correctly clears pending timeouts (line 115) before hiding the tooltip, ensuring tooltips don't appear after the user has moved away from the trigger element.
70-73: LGTM! Cleanup on destroy.The
ngOnDestroyhook properly cleans up both pending timeouts and the tooltip component, preventing memory leaks.projects/element-ng/tooltip/si-tooltip.directive.spec.ts (3)
18-18: LGTM! Removed obsolete triggers binding.Correctly removes the
[triggers]="triggers"binding, aligning the test template with the API change that removed thetriggersinput.
45-58: LGTM! Focus test correctly uses immediate timing.The test properly verifies immediate tooltip display on focus by using
tick(0)(line 48) followed bydetectChanges()(line 49), which accurately reflects the new behavior where focus shows tooltips without delay.
69-79: LGTM! Hover test correctly validates delay behavior.The test properly verifies the 500ms hover delay by:
- Asserting the tooltip is not shown immediately (line 72)
- Ticking 500ms to trigger the delayed display (line 74)
This accurately tests the debounced hover behavior.
api-goldens/element-ng/tooltip/index.api.md (1)
1-44: Skipping review of auto-generated API report.This file is auto-generated by API Extractor and should not be manually reviewed. The changes reflect the removal of the
triggersinput and addition of theshowDelayinput as documented in the PR objectives.src/app/examples/si-tooltip/si-tooltip.html (1)
1-113: LGTM! Well-organized example structure.The reorganized template effectively demonstrates the tooltip directive functionality with clear sections for basic usage, placement options, and advanced examples. The semantic HTML structure with proper ARIA labels on button groups enhances the demonstration quality.
|
Documentation. Coverage Reports: |
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
3928bbd to
cf1d645
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/element-ng/tooltip/si-tooltip.directive.spec.ts (1)
115-138: Inconsistent delay used for focus-triggered tests.These template tests use
tick(500)after focus events, but the directive'sfocusIn()callsshowTooltip(true)which uses a 0ms delay (immediate path). This is inconsistent with the first test block which correctly usestick(0)for focus.While
tick(500)works (it includes elapsed time beyond 0ms), it doesn't accurately test the immediate display behavior and could mask regressions if focus timing changes.🔎 Proposed fix to use tick(0) for focus events
it('should render the template', async () => { button.dispatchEvent(new Event('focus')); - jasmine.clock().tick(500); + jasmine.clock().tick(0); await fixture.whenStable(); expect(document.querySelector('.tooltip')?.innerHTML).toContain('Template content'); button.dispatchEvent(new Event('focusout')); jasmine.clock().tick(500); await fixture.whenStable(); expect(document.querySelector('.tooltip')).toBeFalsy(); }); it('should render the template with context', async () => { fixture.componentInstance.tooltipContext = { tooltip: 'test' }; fixture.changeDetectorRef.markForCheck(); fixture.detectChanges(); button.dispatchEvent(new Event('focus')); - jasmine.clock().tick(500); + jasmine.clock().tick(0); await fixture.whenStable(); expect(document.querySelector('.tooltip')?.innerHTML).toContain('Template content test'); button.dispatchEvent(new Event('focusout')); jasmine.clock().tick(500); await fixture.whenStable(); expect(document.querySelector('.tooltip')).toBeFalsy(); });
♻️ Duplicate comments (1)
src/app/examples/si-tooltip/si-tooltip.ts (1)
16-16: Inconsistent: non-null assertion contradicts defensive check.Line 16 uses the non-null assertion operator (
!), which tells TypeScript thatfocusButtonwill always be defined. However, line 22 includes a defensive checkif (!this.focusButton), which contradicts this assertion.🔎 Recommended: Make the property optional
- @ViewChild('focusButton') focusButton!: ElementRef<HTMLButtonElement>; + @ViewChild('focusButton') focusButton?: ElementRef<HTMLButtonElement>;This accurately reflects that the template reference might not be available and aligns with the defensive check on line 22.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (10)
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (11)
api-goldens/element-ng/tooltip/index.api.mdplaywright/e2e/element-examples/si-tooltip.spec.tsplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yamlprojects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.tssrc/app/examples/si-tooltip/si-tooltip.htmlsrc/app/examples/si-tooltip/si-tooltip.ts
🧰 Additional context used
🧠 Learnings (21)
📚 Learning: 2025-12-11T10:09:01.564Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts:83-104
Timestamp: 2025-12-11T10:09:01.564Z
Learning: In the auto-collapsable-list directive tests (projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts), jasmine.clock() successfully controls the directive's setTimeout-based layout logic in zoneless mode. Mock timers work fine here, unlike in select component overflow tests where real setTimeout waits are required.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tsplaywright/e2e/element-examples/si-tooltip.spec.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-08T11:25:51.584Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.584Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-08T11:24:45.272Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.272Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tsplaywright/e2e/element-examples/si-tooltip.spec.ts
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.tsplaywright/e2e/element-examples/si-tooltip.spec.tssrc/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-16T16:19:17.950Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1207
File: playwright/snapshots/si-filtered-search.spec.ts-snapshots/si-filtered-search--si-filtered-search-playground--data-entered.yaml:38-38
Timestamp: 2025-12-16T16:19:17.950Z
Learning: Do not review files in the playwright/snapshots directory, as they contain auto-generated test artifacts. Exclude any YAML snapshot files under playwright/snapshots from code reviews in the siemens/element repository. If a YAML file is not an auto-generated snapshot, verify its origin before skipping.
Applied to files:
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yaml
📚 Learning: 2025-12-23T09:24:35.163Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/translate/index.api.md:12-84
Timestamp: 2025-12-23T09:24:35.163Z
Learning: In the siemens/element repository, do not review files under the api-goldens/ directory (e.g., api-goldens/**/index.api.md) since they are auto-generated by API Extractor. Exclude these from review checks and focus on source files that are not auto-generated.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-23T09:24:57.843Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/index.api.md:324-325
Timestamp: 2025-12-23T09:24:57.843Z
Learning: Do not review files in the api-goldens directory, as they are auto-generated API reports produced by API Extractor.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-23T09:24:40.395Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/module-federation/index.api.md:7-11
Timestamp: 2025-12-23T09:24:40.395Z
Learning: In the siemens/element repository, do not review any files under the api-goldens directory, as they are auto-generated API reports produced by API Extractor. These MD files should be excluded from review unless explicitly overridden.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-22T13:04:20.883Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: src/app/examples/ag-grid/ag-grid-empty-state.ts:20-24
Timestamp: 2025-12-22T13:04:20.883Z
Learning: The SiEmptyStateComponent in siemens/element-ng accepts icon names in kebab-case format (e.g., 'element-technical-operator') that may not be directly exported as constants in the element-icons.ts file. Do not flag these as errors if they render correctly at runtime.
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-11-18T12:37:30.510Z
Learnt from: Killusions
Repo: siemens/element PR: 1040
File: projects/element-ng/chat-messages/si-chat-input.component.ts:338-349
Timestamp: 2025-11-18T12:37:30.510Z
Learning: In projects/element-ng/chat-messages/si-chat-input.component.ts, the interrupt behavior is intentionally different for button clicks vs Enter key: the button can emit interrupt even without content, but pressing Enter requires content or attachments (canSend() must be true) to emit interrupt.
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-17T08:58:19.300Z
Learnt from: akashsonune
Repo: siemens/element PR: 567
File: projects/charts-ng/src/components/si-chart/si-chart.component.ts:604-609
Timestamp: 2025-12-17T08:58:19.300Z
Learning: In projects/charts-ng/src/components/si-chart/si-chart.component.ts, when implementing theme changes with chart.setTheme() that reuses the ECharts instance without disposing it, do not call afterChartInit() again as it will duplicate event listener registrations, leading to memory leaks and multiple event firings.
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-11T14:44:11.278Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1185
File: projects/element-ng/navbar/si-navbar-item/si-navbar-item.component.html:11-11
Timestamp: 2025-12-11T14:44:11.278Z
Learning: The si-icon component in siemens/element-ng/icon automatically applies aria-hidden="true" to its content. Do not suggest adding aria-hidden="true" to si-icon elements, as accessibility is already handled internally by the component.
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
📚 Learning: 2025-12-09T14:33:54.441Z
Learnt from: Killusions
Repo: siemens/element PR: 998
File: src/app/examples/si-chat-messages/si-ai-message.ts:24-43
Timestamp: 2025-12-09T14:33:54.441Z
Learning: In src/app/examples/ (example/demo code) directories, allow and expect code duplication. Treat duplication as acceptable for demonstration code that aims to illustrate usage rather than enforce DRY. Do not enforce refactoring to remove duplication in example files; focus on readability and clarity of examples. This applies to all TypeScript files under src/app/examples (including nested subdirectories like si-chat-messages).
Applied to files:
src/app/examples/si-tooltip/si-tooltip.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (15)
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yaml (1)
1-26: Skipping auto-generated snapshot file.Based on learnings, files in
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.playwright/e2e/element-examples/si-tooltip.spec.ts (1)
10-18: LGTM! Test correctly updated for focus-based activation.The change from
mouseentertofocus()aligns well with the directive's new behavior where focus triggers immediate tooltip display. The addition of 'auto' to the directions array matches the expanded placement options.playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yaml (1)
1-26: Skipping auto-generated snapshot file.Based on learnings, files in
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yaml (1)
1-26: Skipping auto-generated snapshot file.Based on learnings, files in
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yaml (1)
1-26: Skipping auto-generated snapshot file.Based on learnings, files in
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yaml (1)
1-26: Skipping auto-generated snapshot file.Based on learnings, files in
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.projects/element-ng/tooltip/si-tooltip.directive.spec.ts (2)
45-58: LGTM! Correctly tests immediate display on focus.The use of
tick(0)correctly verifies that focus triggers immediate tooltip display (0ms delay path).
69-79: LGTM! Correctly validates the 500ms hover delay.The test properly verifies that the tooltip is not shown before the delay and appears after the 500ms tick.
projects/element-ng/tooltip/si-tooltip.directive.ts (4)
45-49: LGTM! Clean addition ofshowDelayinput.Good default of 500ms for hover delay, providing a configurable option for consumers while maintaining reasonable default behavior.
70-80: LGTM! Proper timeout cleanup.The
clearShowTimeoutmethod and its usage inngOnDestroyensures no memory leaks from pending timeouts.
82-100: Well-structured debounce implementation.The use of nullish assignment (
??=) for lazy tooltip creation is clean, and the immediate flag correctly differentiates focus (0ms) from hover (configurable delay) behavior.One edge case to be aware of: if
mouseenterfires immediately afterfocusbut before the 0ms timeout executes, it would clear the focus timeout and reschedule with 500ms delay. In practice this is unlikely since the 0ms timeout typically executes in the same event loop tick, but worth noting for future reference.
112-122: LGTM! Consistent hide behavior.Both
hide()andmouseOut()properly clear pending timeouts before hiding, preventing stale callbacks from showing the tooltip after the user has moved away.src/app/examples/si-tooltip/si-tooltip.html (1)
1-154: LGTM! Well-structured example demonstrating tooltip usage.The HTML demonstrates the refactored tooltip directive effectively:
- Properly grouped sections with semantic structure and clear headings
- Comprehensive accessibility with role attributes and aria-label on all interactive elements
- Clear examples of inline tooltips, template-based tooltips, placement options, and advanced use cases (tabs, long text)
- The template references are correctly defined at the end of the file
The use of
[innerHtml]binding (line 148) with thehtmlvariable is safe in this example context since it's a static string.src/app/examples/si-tooltip/si-tooltip.ts (2)
21-27: LGTM! Lifecycle management implemented correctly.The
ngAfterViewInitimplementation properly:
- Includes a defensive check before accessing the element (line 22)
- Stores the timeout reference for cleanup (line 24)
- Demonstrates the tooltip behavior on focus
The previous review comments have been addressed with the addition of proper timeout management.
29-33: LGTM! Proper cleanup in OnDestroy.The timeout cleanup correctly addresses the previous review comment about resource management. The conditional check ensures safe cleanup even if the timeout hasn't been set.
1 task
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
cf1d645 to
c330e56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
projects/element-ng/tooltip/si-tooltip.directive.spec.ts (1)
115-138: Fix timing inconsistency in template tests.The template tests use
tick(500)after focus events (lines 117, 131), but focus events should trigger immediate display withtick(0)as demonstrated in the "with text" tests (line 48). This inconsistency will cause these tests to fail or validate incorrect behavior.🐛 Proposed fix for consistent focus timing
it('should render the template', async () => { button.dispatchEvent(new Event('focus')); - jasmine.clock().tick(500); + jasmine.clock().tick(0); await fixture.whenStable(); expect(document.querySelector('.tooltip')?.innerHTML).toContain('Template content'); button.dispatchEvent(new Event('focusout')); jasmine.clock().tick(500); await fixture.whenStable(); expect(document.querySelector('.tooltip')).toBeFalsy(); }); it('should render the template with context', async () => { fixture.componentInstance.tooltipContext = { tooltip: 'test' }; fixture.changeDetectorRef.markForCheck(); fixture.detectChanges(); button.dispatchEvent(new Event('focus')); - jasmine.clock().tick(500); + jasmine.clock().tick(0); await fixture.whenStable(); expect(document.querySelector('.tooltip')?.innerHTML).toContain('Template content test'); button.dispatchEvent(new Event('focusout')); jasmine.clock().tick(500); await fixture.whenStable(); expect(document.querySelector('.tooltip')).toBeFalsy(); });
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (10)
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (9)
api-goldens/element-ng/tooltip/index.api.mdplaywright/e2e/element-examples/si-tooltip.spec.tsplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yamlprojects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.ts
🧰 Additional context used
🧠 Learnings (18)
📓 Common learnings
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts:83-104
Timestamp: 2025-12-11T10:09:01.564Z
Learning: In the auto-collapsable-list directive tests (projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts), jasmine.clock() successfully controls the directive's setTimeout-based layout logic in zoneless mode. Mock timers work fine here, unlike in select component overflow tests where real setTimeout waits are required.
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.272Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
📚 Learning: 2025-12-16T16:19:17.950Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1207
File: playwright/snapshots/si-filtered-search.spec.ts-snapshots/si-filtered-search--si-filtered-search-playground--data-entered.yaml:38-38
Timestamp: 2025-12-16T16:19:17.950Z
Learning: Do not review files in the playwright/snapshots directory, as they contain auto-generated test artifacts. Exclude any YAML snapshot files under playwright/snapshots from code reviews in the siemens/element repository. If a YAML file is not an auto-generated snapshot, verify its origin before skipping.
Applied to files:
playwright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--auto.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--end.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--start.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--top.yamlplaywright/snapshots/si-tooltip.spec.ts-snapshots/si-tooltip--si-tooltip--bottom.yaml
📚 Learning: 2025-12-23T09:24:35.163Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/translate/index.api.md:12-84
Timestamp: 2025-12-23T09:24:35.163Z
Learning: In the siemens/element repository, do not review files under the api-goldens/ directory (e.g., api-goldens/**/index.api.md) since they are auto-generated by API Extractor. Exclude these from review checks and focus on source files that are not auto-generated.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-23T09:24:57.843Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/index.api.md:324-325
Timestamp: 2025-12-23T09:24:57.843Z
Learning: Do not review files in the api-goldens directory, as they are auto-generated API reports produced by API Extractor.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-23T09:24:40.395Z
Learnt from: spike-rabbit
Repo: siemens/element PR: 1235
File: api-goldens/dashboards-ng/module-federation/index.api.md:7-11
Timestamp: 2025-12-23T09:24:40.395Z
Learning: In the siemens/element repository, do not review any files under the api-goldens directory, as they are auto-generated API reports produced by API Extractor. These MD files should be excluded from review unless explicitly overridden.
Applied to files:
api-goldens/element-ng/tooltip/index.api.md
📚 Learning: 2025-12-11T10:09:01.564Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts:83-104
Timestamp: 2025-12-11T10:09:01.564Z
Learning: In the auto-collapsable-list directive tests (projects/element-ng/auto-collapsable-list/si-auto-collapsable-list.directive.spec.ts), jasmine.clock() successfully controls the directive's setTimeout-based layout logic in zoneless mode. Mock timers work fine here, unlike in select component overflow tests where real setTimeout waits are required.
Applied to files:
playwright/e2e/element-examples/si-tooltip.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T10:05:59.100Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1193
File: projects/element-ng/search-bar/si-search-bar.component.spec.ts:21-28
Timestamp: 2025-12-15T10:05:59.100Z
Learning: In tests that rely on Jasmine's fake clock to control time-based RxJS operators (e.g., debounceTime), use jasmine.clock().install() together with jasmine.clock().mockDate() to enable precise timing control. Without mockDate, scheduler-based operators may not respond to the fake clock. Apply this guidance to spec files (e.g., any file named *.spec.ts or *.spec.js) that test time-sensitive behavior, ensuring you install and mockDate before advancing time in tests.
Applied to files:
playwright/e2e/element-examples/si-tooltip.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-17T04:34:55.597Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1173
File: src/app/examples/si-charts/interactive/interactive.ts:5-5
Timestamp: 2025-12-17T04:34:55.597Z
Learning: Guideline: In Angular projects using OnPush change detection (zoneless mode), you only need to call ChangeDetectorRef.markForCheck() for asynchronous operations (e.g., setTimeout, setInterval, Promises, Observables) where a change is not automatically detected. Event handlers triggered by Angular template bindings (click, input, etc.) will trigger change detection automatically and do not require explicit markForCheck() calls. Apply this broadly to TS files within Angular components/services that use OnPush; use markForCheck() selectively for async work where change detection wouldn’t run otherwise.
Applied to files:
playwright/e2e/element-examples/si-tooltip.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.ts
📚 Learning: 2025-12-15T07:16:53.762Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:92-105
Timestamp: 2025-12-15T07:16:53.762Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits must be used instead of fixture.whenStable() to avoid injector destroyed errors during mount/unmount operations in zoneless mode. The GridStack timing-dependent operations require actual async delays.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-08T11:25:51.584Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:511-516
Timestamp: 2025-12-08T11:25:51.584Z
Learning: In the select component overflow tests (projects/element-ng/select/si-select.component.spec.ts), jasmine.clock() cannot be used to control the timing for overflow item detection. Real setTimeout waits must be used instead, likely due to ResizeObserver or debounced layout calculations that jasmine.clock() cannot control in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-08T11:24:45.272Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts:67-85
Timestamp: 2025-12-08T11:24:45.272Z
Learning: In select lazy options tests (projects/element-ng/select/options/si-select-lazy-options.directive.spec.ts), jasmine.clock() cannot control RxJS debounceTime in zoneless mode. Real setTimeout waits must be used instead for tests involving search debouncing.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T07:17:06.981Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts:37-49
Timestamp: 2025-12-15T07:17:06.981Z
Learning: In widget-instance-editor-dialog component tests (projects/dashboards-ng/src/components/widget-instance-editor-dialog/si-widget-instance-editor-dialog.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing component initialization. This is likely due to the component's lifecycle behavior or modal initialization timing that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-15T07:16:32.082Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1191
File: projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts:69-80
Timestamp: 2025-12-15T07:16:32.082Z
Learning: In gridstack-wrapper component tests (projects/dashboards-ng/src/components/gridstack-wrapper/si-gridstack-wrapper.component.spec.ts), real setTimeout waits (e.g., `await new Promise(resolve => setTimeout(resolve, 0))`) must be used instead of `fixture.whenStable()` when testing grid item rendering and initialization. This is likely due to gridstack library's initialization timing or lifecycle behavior that fixture.whenStable() cannot properly wait for in zoneless mode.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-05T08:00:38.407Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1151
File: projects/element-ng/typeahead/si-typeahead.directive.spec.ts:114-121
Timestamp: 2025-12-05T08:00:38.407Z
Learning: In the typeahead test suite (projects/element-ng/typeahead/si-typeahead.directive.spec.ts), jasmine.clock() must be installed in beforeAll and uninstalled in afterAll (not beforeEach/afterEach) because using per-test clock installation causes schedulerFunction-related issues in these specific tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-01T04:01:27.365Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1099
File: projects/element-ng/accordion/si-accordion.component.spec.ts:39-43
Timestamp: 2025-12-01T04:01:27.365Z
Learning: With Angular 20 and esbuild, synchronous `TestBed.configureTestingModule(...).compileComponents()` calls work correctly even when components use external templates (templateUrl), because esbuild pre-compiles templates at build time. The async/await pattern is not required in this scenario.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-04T05:50:17.637Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:953-964
Timestamp: 2025-12-04T05:50:17.637Z
Learning: In the siemens/element repository, when making tree-view tests zoneless-ready, jasmine.clock() should be used to mimic setTimeouts even in zoneless mode, as the component implementation relies on setTimeout calls that need to be controlled in tests.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-01T14:12:11.111Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1104
File: projects/element-ng/datepicker/components/si-day-selection.component.spec.ts:198-202
Timestamp: 2025-12-01T14:12:11.111Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), prefer `await fixture.whenStable()` over `fixture.detectChanges()` after dispatching events or triggering async operations, as `whenStable()` waits for all asynchronous operations to complete and better mimics the production environment.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.ts
📚 Learning: 2025-12-30T13:52:33.581Z
Learnt from: dauriamarco
Repo: siemens/element PR: 1221
File: projects/element-ng/side-panel/si-side-panel.component.ts:136-142
Timestamp: 2025-12-30T13:52:33.581Z
Learning: Maintain the existing negative naming convention (e.g., disableBackdrop) across the siemens/element repository for consistency. Do not refactor to positive names (e.g., showBackdrop, hasBackdrop) unless the project explicitly changes the convention. In reviews, flag changes that alter established naming patterns in related files under projects/element-ng.
Applied to files:
projects/element-ng/tooltip/si-tooltip.directive.spec.tsprojects/element-ng/tooltip/si-tooltip.directive.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (8)
playwright/e2e/element-examples/si-tooltip.spec.ts (2)
10-10: LGTM! Addition of 'auto' placement testing.The addition of 'auto' to the placement directions ensures comprehensive test coverage for the new Auto placement option introduced in the refactored tooltip examples.
14-14: LGTM! Improved test interaction pattern.The switch from dispatching a mouseenter event to using
.focus()is cleaner and better aligns with accessibility-first testing practices. This change properly reflects the standardized hover/focus behavior described in the PR objectives.projects/element-ng/tooltip/si-tooltip.directive.spec.ts (3)
18-20: LGTM!The template simplification correctly removes the
triggersinput binding, aligning with the removal of this feature from the directive.
45-58: LGTM!The focus test correctly validates the new immediate display behavior using
tick(0)to process thesetTimeout(0)call from the implementation.
69-79: LGTM!The hover test correctly validates the 500ms debounced display behavior with a pre-assertion that the tooltip is not shown immediately.
projects/element-ng/tooltip/si-tooltip.directive.ts (3)
59-73: LGTM!The timeout management is well-implemented:
- Private
showTimeoutfield properly tracks pending display operationsclearShowTimeout()prevents race conditions by canceling stale timeouts- Cleanup in
ngOnDestroy()ensures no memory leaks
75-93: LGTM!The debounced display logic correctly implements the new accessibility guidelines:
- Immediate display (0ms) for focus events ensures keyboard accessibility
- Debounced display (500ms) for hover reduces tooltip flicker during mouse movement
- Clearing existing timeouts before scheduling new ones prevents race conditions
95-111: LGTM!The event handlers correctly map interactions to display behavior:
- Focus triggers immediate display for keyboard users
- Mouseenter uses debounced display for hover interactions
- All hide triggers (touchstart, focusout, mouseleave) properly clear pending timeouts
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
3 times, most recently
from
107d3de to
7391d89
Compare
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
7391d89 to
7a2ce63
Compare
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
7a2ce63 to
2976154
Compare
Member
Author
|
@spike-rabbit I’ve moved the tooltip support needed by the components into a separate PR #1379, so we can focus first on the tooltip refactoring according to the new guidelines. Later on, I can further split the component-specific changes into smaller PRs if you’d prefer. |
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
2976154 to
e5c070d
Compare
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
| <button | ||
| type="button" | ||
| class="btn btn-icon btn-secondary" | ||
| siTooltip="Bottom" |
Member
There was a problem hiding this comment.
Could we use a more descriptive texts e.g. Tooltip placed at the bottom for the tooltips?
Member
Author
There was a problem hiding this comment.
I’d prefer to keep the labels short so they’re less affected by the available space around them and can position correctly more consistently. The "Tooltip placements" section title should already provide enough context, I think.
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
spliffone
reviewed
Jan 27, 2026
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
2 times, most recently
from
635c6ee to
52e8545
Compare
spliffone
reviewed
Jan 27, 2026
| protected readonly arrowPos = signal<OverlayArrowPosition | undefined>(undefined); | ||
| protected readonly isVisible = signal(true); | ||
| /** @defaultValue true */ | ||
| readonly isOpening = signal(true); |
Member
There was a problem hiding this comment.
We should mark this property as internal?
spliffone
reviewed
Jan 27, 2026
| } | ||
|
|
||
| /** @internal */ | ||
| onTransitionstart(event: TransitionEvent): void { |
Member
There was a problem hiding this comment.
Can we set this handler to protected?
spliffone
reviewed
Jan 27, 2026
| } | ||
|
|
||
| /** @internal */ | ||
| onTransitionend(event: TransitionEvent): void { |
Member
There was a problem hiding this comment.
I guess this method should be protected too
spliffone
reviewed
Jan 27, 2026
| constructor() { | ||
| afterNextRender(() => { | ||
| const button = this.focusButton(); | ||
| if (!button) return; |
Member
There was a problem hiding this comment.
Suggested change
| if (!button) return; |
spliffone
reviewed
Jan 27, 2026
| const button = this.focusButton(); | ||
| if (!button) return; | ||
|
|
||
| this.focusTimeout = setTimeout(() => { |
Member
There was a problem hiding this comment.
Since we use the render next callback I pretty sure we don't need the setTimeout nor the onDestroy anymore
spike-rabbit
requested changes
Jan 27, 2026
| [id]="id()" | ||
| [class]="tooltipPositionClass()" | ||
| > | ||
| @if (isVisible()) { |
Member
There was a problem hiding this comment.
do we really need this? In general I would prefer that the component itself is only rendered in the DOM if it supposed to be visible.
Member
Author
There was a problem hiding this comment.
I think I had added it to use the animate API, but I'll try to refactor it 👍
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
52e8545 to
3ead60a
Compare
Member
|
@dauriamarco @spike-rabbit I love how is going! But im now having second thoughts about the timing...im following standard best practinces...but do you guys feel is too fast? Should we delay a bit more? |
Member
Author
@panch1739 To me it feels quite right. In a real-world context it would probably feel different than in the examples, where everything is shown together and we keep interacting with it. |
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
2 times, most recently
from
08f6a70 to
0809648
Compare
spike-rabbit
requested changes
Jan 29, 2026
| } | ||
| } | ||
|
|
||
| .tooltip-wrapper { |
Member
There was a problem hiding this comment.
I don't like having this extra wrapper in the style sheets. Maybe have the animations as before, and just add a dummy transition to root which slows down the removal.
The element-theme should ideally not contain angular specific workarounds.
Remove dismissible tooltip functionality and standardize interaction patterns. Tooltips now follow consistent hover/focus behavior as per accessibility guidelines. BREAKING CHANGE: removed `triggers` input The `triggers` input has been removed to standardize tooltip behavior according to accessibility guidelines. If you were using `triggers="focus"` to create dismissible tooltips, remove the `triggers` attribute as this functionality is no longer supported.
dauriamarco
force-pushed
the
refactor/new-tooltip-guidelines
branch
from
0809648 to
754fe66
Compare
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Remove dismissible tooltip functionality and standardize interaction patterns. Tooltips now follow consistent hover/focus behavior as per accessibility guidelines.
BREAKING CHANGE: removed
triggersinputThe
triggersinput has been removed to standardize tooltip behavior according to accessibility guidelines. If you were usingtriggers="focus"to create dismissible tooltips, remove thetriggersattribute as this functionality is no longer supported.Tooltip Refactor: Accessibility-Aligned Guidelines
This PR refactors SiTooltipDirective to follow accessibility guidelines by removing dismissible tooltips and standardizing hover/focus behavior.
Key changes:
triggersinput (consumers usingtriggers="focus"must remove it).triggersfrom the public inputs.Migration note: Remove any use of the
triggersattribute — dismissible/focus-triggered tooltips are no longer supported.