refactor(navbar-vertical): optimize notifications/badges visibility logic #1226
Conversation
📝 WalkthroughWalkthroughConsolidates item markup into an Sequence Diagram(s)(omitted) Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (12)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (9)
playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yaml(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html(2 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss(3 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts(5 hunks)src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts(3 hunks)src/app/examples/si-navbar-vertical/si-navbar-vertical.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (14)
📓 Common learnings
Learnt from: spike-rabbit
Repo: siemens/element PR: 1220
File: projects/element-ng/filtered-search/si-filtered-search.component.spec.ts:2602-2733
Timestamp: 2025-12-19T10:37:49.810Z
Learning: For the siemens/element repository, when reviewing PRs, do not duplicate or re-report issues that are already mentioned in the PR comments or objectives. Focus on new findings from the code changes only.
📚 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:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 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/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yaml
📚 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-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.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:
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
📚 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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-04T05:50:38.735Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1135
File: projects/element-ng/tree-view/si-tree-view.component.spec.ts:1244-1245
Timestamp: 2025-12-04T05:50:38.735Z
Learning: In zoneless Angular tests with OnPush components, calling `await runOnPushChangeDetection(fixture)` followed by `await fixture.whenStable()` is NOT redundant. The `runOnPushChangeDetection` helper forces change detection to run for OnPush components, while `whenStable()` waits for all async operations to complete. Both are necessary and serve different purposes.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.861Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.861Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
🧬 Code graph analysis (1)
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (1)
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (1)
Component(15-133)
⏰ 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 (9)
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (1)
24-28: LGTM! Clean addition of example menu item.The new
MenuItemComponent, route, and menu item data are well-structured and appropriate for demonstration purposes. The menu item properly includes bothbadgeandbadgeColorproperties, making it a good example of the refactored badge functionality.Also applies to: 71-74, 113-121
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts (1)
155-233: LGTM! Comprehensive test updates for renamed badge classes.The test suite has been properly updated to reflect the class name change from
.badge-textto.badge-collapsed. All selectors and assertions are correctly updated, and the test coverage remains comprehensive.Note: The test at line 171-182 confirms that badges can be set without
badgeColor, which validates the concern raised in the template review about handling undefinedbadgeColorvalues.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss (3)
31-38: LGTM! Proper visibility logic for expanded badge state.The visibility rules correctly hide
.badge-expandedwhen the parent has the.showclass and adjust the dropdown caret margins appropriately. This aligns well with the template changes.
85-91: LGTM! Correct visibility toggling between collapsed and expanded states.The selectors properly hide
.badge-expandedin collapsed navigation and hide.badge-collapsedin expanded navigation. This ensures the appropriate badge variant is shown based on the navbar state.
132-144: LGTM! Well-defined styling for collapsed badge.The sizing (14px × 14px minimum), typography (caption size, semibold weight), and spacing are appropriate for a compact badge indicator in collapsed state. The circular appearance (7px border-radius on 14px height) works well for notification badges.
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (4)
112-115: Simplified action handler is cleaner.The action handler no longer mutates
item.active, which simplifies the logic and aligns well with the comment discouraging the use oftype: 'action'. The parameterless arrow function is cleaner and focuses on the essential callback behavior.
40-45: No issues found. The new menu item withrouterLink: 'menu-item'references a properly configured route that is defined in the routing configuration with the correct component.
97-123: Routes 'coverage' and 'energy' are properly defined in the routing configuration atsrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tswith their corresponding components (TestCoverageComponent and EnergyComponent).
35-123: Badge colors and badge type support are correctly implemented.The
NavbarVerticalItemcomponent properly supports both string and numeric badge values. The badge colors used in the code—'default', 'danger-emphasis', 'info', 'warning', 'info-emphasis', 'danger', and 'success'—are all valid values defined in the iX design system's badge color type. The component correctly applies the 'bg-' prefix in the template as specified in the documentation.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
Outdated
Show resolved
Hide resolved
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
eaf0770 to
5931d9e
Compare
|
Documentation. Coverage Reports: |
|
@dauriamarco Awesomeeee! Thank you so much. Just two things:
Could you check this things? 🙏 Thank you so much! |
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
2 times, most recently
from
fd5ecdb to
10a1306
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (16)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (15)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yaml(1 hunks)playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yaml(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts(2 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss(5 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts(6 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts(2 hunks)src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts(3 hunks)src/app/examples/si-navbar-vertical/si-navbar-vertical.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: spike-rabbit
Repo: siemens/element PR: 1220
File: projects/element-ng/filtered-search/si-filtered-search.component.spec.ts:2602-2733
Timestamp: 2025-12-19T10:37:49.810Z
Learning: For the siemens/element repository, when reviewing PRs, do not duplicate or re-report issues that are already mentioned in the PR comments or objectives. Focus on new findings from the code changes only.
📚 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/navbar-vertical/si-navbar-vertical.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yaml
📚 Learning: 2025-12-17T14:00:55.647Z
Learnt from: dauriamarco
Repo: siemens/element PR: 908
File: projects/element-ng/side-panel/si-side-panel.component.ts:70-74
Timestamp: 2025-12-17T14:00:55.647Z
Learning: In the Element side-panel component (projects/element-ng/side-panel/si-side-panel.component.ts), the `xxxlMinimum` breakpoint (1920px) is intentionally hardcoded as a private static constant rather than being part of the configurable `Breakpoints` interface. This design preserves full Bootstrap compatibility (which only defines breakpoints up to xxl/1400px) while allowing Element to handle ultra-wide screens explicitly without affecting the standard breakpoint configuration.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss
📚 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:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 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-navbar-vertical/si-navbar-vertical.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts
📚 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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.861Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.861Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
🧬 Code graph analysis (1)
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (2)
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (1)
Component(15-133)src/app/examples/si-navbar-vertical/si-navbar-vertical-text.ts (1)
Component(20-60)
⏰ 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 (20)
projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss (1)
135-137: LGTM! Search button layout optimized for collapsed state.The change from
display: nonetodisplay: flexwith full width and flex-start alignment appropriately accommodates the search button in the collapsed navbar state. The flexbox layout provides proper alignment for the icon-only button.projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts (2)
147-156: Collapse animation coordinates title fade-out with badge fade-in.The collapse transition appropriately fades out the item title (0.2s) while fading in the collapsed badge for dropdown toggles (0.5s). The longer badge animation duration aligns with the overall 0.5s collapse animation.
116-125: Animation timing and badge handling are correctly implemented.The expand animation sequence is well-coordinated: icon repositions first (0.1s), then title fades in with a slight delay (0.2s starting at 0.1s). The collapse animation properly fades out the title (0.2s) while fading in the badge-collapsed element (0.5s).
Text-only badges are correctly handled: the
textOnlyBadgecomputed property identifies non-numeric badges, thebadge-text-onlyCSS class hides the text (color: transparent, font-size: 0), and thebadge-collapsedstyling creates a 14px dot appearance when the navbar is collapsed. The animation in the collapse transition properly fades in this dot element.projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts (2)
16-16: Good addition: text-truncate prevents header overflow.Adding
text-truncateto the header container ensures long navigation section titles don't overflow in the collapsed state.
35-48: Well-coordinated header and divider animation timing.The animation refinements create a smooth collapse/expand transition:
- Collapse: Header fades out over 0.25s, then divider fades in starting at 0.25s (sequential)
- Expand: Header fades in starting at 0.1s (earlier than before for quicker appearance)
The more specific selectors (
.si-h5:leaveinstead of:leave) improve animation targeting and prevent unintended element matching.playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yaml (1)
1-19: Skipping auto-generated snapshot file.Based on learnings, playwright snapshot files are auto-generated test artifacts and should not be reviewed.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html (1)
2-8: LGTM!The consolidated
item-contentwrapper with conditional icon rendering and dedicateditem-titleblock is a clean refactor that improves the component structure.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts (1)
68-74: LGTM!The
textOnlyBadgecomputed property correctly identifies non-numeric badges. The logic is sound: it returnstrueonly when a badge exists and is not a number, which aligns with the PR requirement to show text badges as dots in collapsed state.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts (1)
212-238: LGTM!Comprehensive test cases covering the badge formatting logic for various input types. The consistency test ensures collapsed and expanded badges render identically.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss (2)
44-56: LGTM!The active state styling correctly uses
element-action-primary-hoverfor both text color and the indicator bar, addressing the PR review feedback.
161-164: LGTM!The
badge-text-onlystate correctly hides badge text usingcolor: transparentandfont-size: 0while preserving the dot indicator, fulfilling the PR requirement for text badges in collapsed state.src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (3)
24-28: LGTM!The new MenuItemComponent follows the same pattern as other dummy components in this routing example file and is appropriate for demonstration purposes.
71-74: LGTM!The route entry correctly maps the 'menu-item' path to the newly added MenuItemComponent and follows the established routing pattern.
119-136: LGTM!The menu structure updates effectively demonstrate the badge visibility improvements:
- The badge color upgrade to 'danger-emphasis' enhances visibility
- The 'Emergencies' item with text badge 'Hot' tests the text badge behavior mentioned in the PR objectives
- The routing integration is correct
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (6)
35-37: LGTM!The text badge 'New' with 'default' color effectively demonstrates the text badge visibility feature that's central to this PR's objectives.
41-46: LGTM!The Menu item entry correctly aligns with the new route defined in the routing file, and the Modules header provides clear organizational structure for the navigation items.
52-75: LGTM!The addition of badges at both group and child levels effectively tests the nested badge visibility behavior with various severity colors, demonstrating the improved badge rendering logic.
83-93: LGTM!The Documentation group badges demonstrate the info-emphasis color variant and test badge behavior in grouped navigation items.
97-102: LGTM!The Test coverage item without a badge provides a useful baseline for comparing badge rendering behavior in the example.
116-123: LGTM!The Energy & sustainability item with a numeric badge of 100 and 'success' color effectively demonstrates badge visibility for larger numbers and positive states.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
Outdated
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts
Show resolved
Hide resolved
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
10a1306 to
e0e83d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/navbar-vertical/si-navbar-vertical-item.component.spec.ts (1)
160-243: Consider adding test coverage forbadge-text-onlyclass.The test suite covers collapsed badge behavior and textOnly mode, but there's no test verifying that the
badge-text-onlyclass is applied when the badge contains text (non-numeric value). This class is conditionally applied via[class.badge-text-only]="textOnlyBadge()"in the template.🔎 Suggested test case
it('should apply badge-text-only class for non-numeric badges when collapsed', () => { mockNavbar.collapsed.set(true); component.item.set({ type: 'link', label: 'Test', href: '#', icon: 'element-test', badge: 'new', // text badge badgeColor: 'info' }); fixture.detectChanges(); const badgeElement = fixture.nativeElement.querySelector('.badge'); expect(badgeElement).toBeTruthy(); expect(badgeElement.classList.contains('badge-text-only')).toBe(true); }); it('should not apply badge-text-only class for numeric badges', () => { mockNavbar.collapsed.set(true); component.item.set({ type: 'link', label: 'Test', href: '#', icon: 'element-test', badge: 5, // numeric badge badgeColor: 'info' }); fixture.detectChanges(); const badgeElement = fixture.nativeElement.querySelector('.badge'); expect(badgeElement).toBeTruthy(); expect(badgeElement.classList.contains('badge-text-only')).toBe(false); });
♻️ Duplicate comments (2)
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html (1)
10-17: Remove non-idiomaticthis.prefix in template bindings.Lines 14-15 still use
this.prefix when accessing component members. While functional, this is non-idiomatic in Angular templates.🔎 Proposed fix
@if (!navbar.textOnly() && formattedBadge()) { <span class="badge" [ngClass]="'bg-' + (item().badgeColor ?? 'default')" - [class.badge-text-only]="this.textOnlyBadge()" - [class.badge-collapsed]="this.navbar.collapsed()" + [class.badge-text-only]="textOnlyBadge()" + [class.badge-collapsed]="navbar.collapsed()" >{{ formattedBadge() }}</span > }src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (1)
112-115: ChangebadgeColorfrom'danger'to'danger-emphasis'for consistency.Line 114 uses
badgeColor: 'danger', which is inconsistent with other high-severity badges in this file and in si-navbar-vertical-routing.ts. For example:
- Line 53: User management uses
'danger-emphasis'- Line 67: Sub item 2 uses
'danger-emphasis'- si-navbar-vertical-routing.ts line 129: Emergencies uses
'danger-emphasis'The "Hot" badge on the Action item has similar semantic severity and should follow the same convention.
🔎 Proposed fix
- badgeColor: 'danger' + badgeColor: 'danger-emphasis'
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (16)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (15)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yaml(1 hunks)playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yaml(1 hunks)playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yaml(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts(2 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss(5 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts(6 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss(1 hunks)projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts(2 hunks)src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts(3 hunks)src/app/examples/si-navbar-vertical/si-navbar-vertical.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (17)
📚 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/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yaml
📚 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/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.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:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2025-12-09T14:32:34.036Z
Learnt from: Killusions
Repo: siemens/element PR: 998
File: projects/element-ng/chat-messages/si-ai-chat-container.component.html:21-47
Timestamp: 2025-12-09T14:32:34.036Z
Learning: In projects/element-ng/chat-messages, nested if blocks are preferred over switch statements for branching on message types in Angular templates.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
📚 Learning: 2025-12-17T14:00:55.647Z
Learnt from: dauriamarco
Repo: siemens/element PR: 908
File: projects/element-ng/side-panel/si-side-panel.component.ts:70-74
Timestamp: 2025-12-17T14:00:55.647Z
Learning: In the Element side-panel component (projects/element-ng/side-panel/si-side-panel.component.ts), the `xxxlMinimum` breakpoint (1920px) is intentionally hardcoded as a private static constant rather than being part of the configurable `Breakpoints` interface. This design preserves full Bootstrap compatibility (which only defines breakpoints up to xxl/1400px) while allowing Element to handle ultra-wide screens explicitly without affecting the standard breakpoint configuration.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss
📚 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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.861Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.861Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.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-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.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 (23)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yaml (1)
1-26: Skipping auto-generated snapshot file.Based on learnings, Playwright snapshot files under
playwright/snapshotsare auto-generated test artifacts and should be excluded from code review.projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss (1)
134-138: LGTM!The change from
display: blocktodisplay: flexwithjustify-content: flex-startprovides better alignment control for the search button in collapsed state, andinline-size: 100%ensures consistent full-width behavior.playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yaml (1)
1-19: Skipping auto-generated snapshot file.Based on learnings, Playwright snapshot files under
playwright/snapshotsare auto-generated test artifacts and should be excluded from code review.projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts (2)
116-125: Verify:positionproperty is not animatable in CSS.The
positionproperty is discrete and cannot be smoothly interpolated betweenabsoluteandrelative. Angular animations will apply these styles, but the value will snap rather than transition. If the intent is to setposition: absoluteat animation start andposition: relativeat end, this works, but there won't be a smooth visual transition for the position change itself.Consider whether this produces the intended visual effect, or if an alternative approach (e.g., using
transformfor positioning) would be more appropriate.
147-156: LGTM!The opacity animations for fading out
.item-titleand fading in.badge-collapsedduring the collapse transition are well-structured. The timing (0.2s for title fade-out, 0.5s for badge fade-in) provides smooth visual transitions.playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yaml (1)
1-23: Skipping auto-generated snapshot file.Based on learnings, Playwright snapshot files under
playwright/snapshotsare auto-generated test artifacts and should be excluded from code review.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html (1)
2-8: LGTM on the new item-content structure.The consolidated wrapper with conditional icon rendering and dedicated item-title block is well-structured. The
d-flex align-items-centerclasses properly align the content, and thetext-truncateon the title prevents overflow issues.projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts (1)
25-53: LGTM on animation refinements.The updated animation choreography with more specific selectors (
.si-h5:leave,.si-h5:enter) and the staggered divider entrance (0.25s delay) creates a smoother visual transition between collapsed and expanded states. The timing adjustments are reasonable.playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yaml (1)
1-31: Skipping auto-generated snapshot file.Based on learnings, Playwright snapshot files under
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yaml (1)
1-31: Skipping auto-generated snapshot file.Based on learnings, Playwright snapshot files under
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts (1)
68-74: LGTM on the textOnlyBadge computed property.The logic correctly identifies non-numeric badges. The falsy check (
badge ?) handles undefined/null cases, andtypeof badge !== 'number'properly distinguishes string badges. Edge cases like empty strings are handled byformattedBadge()which returns''for falsy values, preventing badge rendering altogether.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts (1)
34-50: LGTM on mock state reset.The
beforeEachblock now properly resetscollapsed,textOnly, anditemTriggeredspy state, ensuring test isolation. This addresses the previous review feedback.playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yaml (1)
1-30: Skipping auto-generated snapshot file.Based on learnings, Playwright snapshot files under
playwright/snapshotsare auto-generated test artifacts and should not be reviewed.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss (3)
19-21: LGTM on item-content container.The
overflow: hiddenon.item-contentproperly contains the icon and title elements, preventing layout issues during collapse/expand animations.
44-56: LGTM on active state styling.The updated active state uses
element-action-primary-hoverfor both text color and the 4px indicator, aligning with the PR objective to update selected item styling.
132-146: LGTM on badge transition properties.The transition now specifies individual properties instead of
all, addressing the previous review feedback about performance. The property list covers the necessary animated properties.src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (3)
24-28: LGTM! Clean dummy component for the example.The new
MenuItemComponentfollows the same pattern as other dummy components in the file and serves its purpose for demonstrating routing.
71-74: LGTM! Route correctly configured.The new route properly maps the 'menu-item' path to the
MenuItemComponent, consistent with the routing structure in this example.
119-136: LGTM! Menu structure demonstrates various navbar features.The updated menu items showcase different item types (headers, router-links with badges) and consistently use
'danger-emphasis'for high-severity badges, which aligns with the example in the other file.src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (4)
35-46: LGTM! Badge additions demonstrate navbar features effectively.The additions showcase text badges (e.g., 'New') with different colors and the use of headers to organize menu sections, which are valuable for demonstration purposes.
52-96: LGTM! Comprehensive badge demonstration across groups and items.The badge additions across user management and documentation groups effectively demonstrate various badge types (numeric), colors (info, danger-emphasis, warning, info-emphasis), and their usage on both parent groups and child items.
116-123: LGTM! Energy item demonstrates success-colored numeric badge.The Energy & sustainability item with badge
100andbadgeColor: 'success'effectively demonstrates how to use positive/success-state badges with larger numeric values.
112-112: Verify the action callback signature change is compatible with NavbarVerticalItem type.The action callback was simplified from accepting an
itemparameter and settingitem.active = trueto a parameterless function. While this may be an intentional simplification for the example, it's important to verify that theNavbarVerticalItemtype definition allows parameterless action callbacks.#!/bin/bash # Description: Check NavbarVerticalItem type definition for action property signature # Search for NavbarVerticalItem type definition ast-grep --pattern $'export type NavbarVerticalItem = $$$' # Also search for the action property definition in interfaces or types rg -nP --type=ts -A5 -B2 'action\??:\s*\(' projects/element-ng/navbar-vertical/
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
Show resolved
Hide resolved
Hey @panch1739 thank you for the feedback! 🙏 I've implemented both changes you requested. I've also took the opportunity to optimize a few other details and the animations. |
|
@dauriamarco thanks! I cannot open the example link...im not sure why: 
I tried with different configurations of my zscaler and access, but i cannot open it...do you know how i can open it? i think we need to work also on better examples, since we now are putting too much emphasis on the badges. I think that is not a super mega feature that we want to propose...is more of a necessary evil for some projects. I know that in the code we can do everything, but i think we should promote best practices or most common cases. This is my proposal for examples:
@kfenner WDYT? |
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
e0e83d3 to
faf7d6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
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/navbar-vertical/si-navbar-vertical-item.component.spec.ts (1)
160-243: Add test coverage for thebadge-text-onlyclass.The template conditionally applies the
badge-text-onlyclass (line 14 of the template) based ontextOnlyBadge(), but there's no explicit test verifying this class is applied when the badge contains text-only (non-numeric) values.🧪 Proposed test case
Add a test case to verify the
badge-text-onlyclass is applied for text badges:it('should apply badge-text-only class for non-numeric badge values', () => { component.item.set({ type: 'link', label: 'Test', href: '#', badge: 'new', badgeColor: 'info' }); fixture.detectChanges(); const badgeElement = fixture.nativeElement.querySelector('.badge'); expect(badgeElement).toBeTruthy(); expect(badgeElement.classList.contains('badge-text-only')).toBe(true); expect(badgeElement.textContent.trim()).toBe('new'); }); it('should not apply badge-text-only class for numeric badge values', () => { component.item.set({ type: 'link', label: 'Test', href: '#', badge: 99, badgeColor: 'info' }); fixture.detectChanges(); const badgeElement = fixture.nativeElement.querySelector('.badge'); expect(badgeElement).toBeTruthy(); expect(badgeElement.classList.contains('badge-text-only')).toBe(false); expect(badgeElement.textContent.trim()).toBe('99'); });
🤖 Fix all issues with AI agents
In @projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts:
- Around line 68-74: Simplify the ternary in the computed property
textOnlyBadge: replace the current badge ? typeof badge !== 'number' : false
logic with a direct boolean expression that checks existence and type, e.g. use
!!this.item().badge && typeof this.item().badge !== 'number' inside the
computed; update the computed body for textOnlyBadge to return that expression
and keep the computed name (textOnlyBadge) and the badge access
(this.item().badge) unchanged.
In @projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss:
- Around line 135-137: The CSS rule in si-navbar-vertical.component.scss
contains a redundant declaration "justify-content: flex-start" inside the flex
container block (alongside "display: flex" and "inline-size: 100%"); remove the
"justify-content: flex-start" line unless you intentionally need to be explicit
or override a global style—if you must keep it for clarity, add a comment
explaining why it's intentional.
In @projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts:
- Around line 116-120: The animation is currently animating the CSS position in
the query('.item-content .icon', ...) block which causes layout thrashing;
change it to keep position static and animate transform and/or opacity instead
(e.g., animate to transform: translateX(0) and opacity: 1 from an offset
translateX value), keeping the { optional: true } flag and the same timing
('0.2s') so the icon is revealed smoothly without changing position from
absolute/relative; update the matching initial style and the animate style in
the same query call and ensure any layout that required absolute positioning
remains unchanged outside the animated properties.
In @src/app/examples/si-navbar-vertical/si-navbar-vertical.ts:
- Around line 112-115: Update the badgeColor value for the "Hot" action item
from 'danger' to 'danger-emphasis' to match the routing file and consistent
high-severity styling; locate the object containing action: () =>
this.logEvent('Callback for action called'), badge: 'Hot', badgeColor: 'danger'
and change badgeColor to 'danger-emphasis'.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (16)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (15)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yamlprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scssprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical.component.scssprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
🧰 Additional context used
🧠 Learnings (23)
📚 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/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yaml
📚 Learning: 2025-12-22T13:04:35.578Z
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:35.578Z
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:
projects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical.component.scssprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 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/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.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/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts
📚 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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.861Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.861Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-17T14:01:05.705Z
Learnt from: dauriamarco
Repo: siemens/element PR: 908
File: projects/element-ng/side-panel/si-side-panel.component.ts:70-74
Timestamp: 2025-12-17T14:01:05.705Z
Learning: In the Element side-panel component (projects/element-ng/side-panel/si-side-panel.component.ts), the `xxxlMinimum` breakpoint (1920px) is intentionally hardcoded as a private static constant rather than being part of the configurable `Breakpoints` interface. This design preserves full Bootstrap compatibility (which only defines breakpoints up to xxl/1400px) while allowing Element to handle ultra-wide screens explicitly without affecting the standard breakpoint configuration.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss
📚 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:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2025-12-22T13:16:19.243Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: projects/element-ng/icon/element-icons.ts:166-167
Timestamp: 2025-12-22T13:16:19.243Z
Learning: In projects/element-ng/icon/element-icons.ts, the elementCheckedImageShape icon intentionally uses base64 encoding instead of utf8 encoding (which is used for other icons). This approach is considered safer and correct for this specific icon and should not be flagged as an inconsistency.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlsrc/app/examples/si-navbar-vertical/si-navbar-vertical.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2025-12-09T14:32:34.036Z
Learnt from: Killusions
Repo: siemens/element PR: 998
File: projects/element-ng/chat-messages/si-ai-chat-container.component.html:21-47
Timestamp: 2025-12-09T14:32:34.036Z
Learning: In projects/element-ng/chat-messages, nested if blocks are preferred over switch statements for branching on message types in Angular templates.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
📚 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-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
📚 Learning: 2025-12-30T13:52:41.363Z
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:41.363Z
Learning: In the siemens/element repository, the library uses negative naming conventions like `disableBackdrop` intentionally for consistency across the codebase. This pattern should be maintained rather than converting to positive naming conventions like `showBackdrop` or `hasBackdrop`.
Applied to files:
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts
📚 Learning: 2025-12-31T07:16:13.451Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: projects/element-ng/ag-grid/parts/radio-style.part.ts:15-61
Timestamp: 2025-12-31T07:16:13.451Z
Learning: In the AG Grid theme package (projects/element-ng/ag-grid), styling should maintain consistency with the base Element design system components rather than adding enhancements or extra CSS. If accessibility improvements like focus-visible indicators are needed, they should be implemented at the Element design system level first.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2026-01-02T06:41:22.404Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: playwright/e2e/element-examples/static.spec.ts:119-121
Timestamp: 2026-01-02T06:41:22.404Z
Learning: In the Element design system (siemens/element repository), disabling the `aria-required-children` accessibility rule for `SiEmptyStateComponent` is an accepted pattern. This component is used in AG Grid overlays (AgNoRowsOverlayComponent) and other places in Element, and the rule is consistently disabled in tests where this component appears (e.g., si-pills-input, datatable-row-group, ag-grid-empty-state).
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
🧬 Code graph analysis (1)
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (1)
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (1)
Component(15-133)
⏰ 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 (14)
projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts (1)
16-16: LGTM! Animation refinements improve specificity and coordination.The changes enhance the header animations:
- Adding
text-truncateprevents overflow issues- More specific selectors (
.si-h5:leave,.si-h5:enter) improve targeting- The divider fade-in at 0.25s aligns perfectly with the h5 fade-out completion
- Adding
ease-outto the group animation provides smoother visual transitionsThese timing adjustments coordinate well with the navbar item animations in the main component.
Also applies to: 35-49
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss (3)
19-21: LGTM! Overflow hidden prevents layout issues.Adding
overflow: hiddento.item-contentproperly contains child elements and prevents layout overflow during animations or when content is truncated.
45-45: LGTM! Active state colors updated per PR feedback.The active state now correctly uses
element-action-primary-hoverfor both the text color and the 4px indicator, which aligns with the PR objectives and panch1739's feedback requesting this specific color for selected items.Also applies to: 54-54
132-146: LGTM! Transition performance improved.The
.badgetransitions now specify individual properties instead oftransition: all, which addresses the previous performance concern. Listing specific properties allows the browser to optimize rendering by only computing the properties that actually animate.playwright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yaml (1)
1-23: Skipping review of auto-generated snapshot file.This Playwright snapshot file is auto-generated during test execution and should not be manually reviewed. The changes reflect the expected UI output based on the navbar component modifications.
Based on learnings, YAML files in the
playwright/snapshotsdirectory are test artifacts that are automatically generated and updated when component behavior changes.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html (1)
14-15: Remove the non-idiomaticthis.prefix in template bindings.Angular templates don't require the
this.prefix when accessing component members. Lines 14-15 still usethis.textOnlyBadge()andthis.navbar.collapsed()which is non-idiomatic.⛔ Skipped due to learnings
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:35.578Z 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.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.Learnt from: Killusions Repo: siemens/element PR: 998 File: projects/element-ng/chat-messages/si-ai-chat-container.component.html:21-47 Timestamp: 2025-12-09T14:32:34.036Z Learning: In projects/element-ng/chat-messages, nested if blocks are preferred over switch statements for branching on message types in Angular templates.src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (3)
24-28: LGTM!The new MenuItemComponent is appropriate for demonstration purposes.
71-74: LGTM!The route definition is correct and follows the established pattern.
119-136: LGTM!The badgeColor change to 'danger-emphasis' addresses previous feedback and ensures consistency. The menu restructuring with the new 'Emergencies' item appropriately demonstrates badge usage.
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (5)
35-37: LGTM!The badge addition demonstrates text badge usage appropriately.
41-44: LGTM!The menu item restructuring aligns with the routing changes.
46-96: LGTM!The badge additions across menu groups and items effectively demonstrate various badge types and colors, consistent with the PR's objective to improve badge visibility.
97-103: LGTM!The repositioning and divider addition improve the logical grouping of menu items.
116-123: LGTM!The Energy & sustainability item with success badge appropriately demonstrates numeric badge usage.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts
Outdated
Show resolved
Hide resolved
Hey @panch1739! This one's not your fault 👍 sometimes PRs just need a quick update after a while and the example links need to be regenerated. The link should now be accessible, so you can try again. Regarding the examples: I agree that the main one is currently a bit cluttered, and we can definitely improve it. I’ll wait for some final feedbacks on that, and in the meantime we can use this one mainly to quickly test all variations in one place. |
@dauriamarco sounds perfect! You are right, lets review all and when we are comfortable with the design and the code, we refine the examples ;) |
@dauriamarco we had a look also with @hbxes and it looks all good from our side 💃 Just one thing came in our discussion...could we add an option to hide the badge in the collapse state of the vertical nav? The issue is that subtle badges (those with soft background colors like base-information or base-danger) don’t read well when collapsed without text, they become too pale. In the previous implementation, this was handled by switching to a brighter color in the collapsed state. During the alignment with iX, however, it was requested that badges keep the same color in both expanded and collapsed states. This works well for strong variants, but not for subtle ones. We don’t want to force projects to hide badges, but we want to give them the option to do so. Basically, the rule would be: Do not overcrowd the vertical navigation with badges. Use at most one badge, and only when it communicates critical information. If a badge is essential, use the emphasis (strong) variant. Subtle badges without a counter (text-only) should preferably not be shown in the collapsed state. |
@panch1739 @hbxes It should be possible to support something like this 👍 Just to make sure: would this option apply per individual |
@dauriamarcoThe logic for hiding badges mainly applies to subtle variants. If a badge is subtle, it already signals that the information is not critical, so hiding it in the collapsed state is acceptable. If the information is important, an emphasis (strong) badge should be used instead and remain visible. Or what do you think? |
Ok, I understand. In that case it shouldn’t be an option left to the consumer to decide but we should enforce the hide behavior ourselves based on the badge variant/class. Conceptually, I see some possible approaches:
I don’t have a strong preference, looping @kfenner and @spike-rabbit in to also get their thoughts. |
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
faf7d6b to
43d962c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html:
- Around line 10-17: The template mixes usage of the `this.` prefix (e.g.,
`this.textOnlyBadge()`, `this.navbar.collapsed()`) with non-prefixed expressions
(`navbar.textOnly()`, `formattedBadge()`, `item().badgeColor`); remove the
`this.` prefix from `this.textOnlyBadge()` and `this.navbar.collapsed()` so all
template bindings consistently use `textOnlyBadge()` and `navbar.collapsed()`
(keep existing calls to `formattedBadge()` and `item().badgeColor` unchanged).
In @projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss:
- Around line 141-156: The .badge selector currently lists many individual
transition properties; simplify by consolidating into a shorter transition
declaration if all share the same timing (e.g., use a single "transition: all
0.5s ease-out" or combine groups with the same duration), or else explicitly
keep only the few properties that need different timings (preserve separate
entries for color and font-size at 0.2s and group the rest at 0.5s) so the
stylesheet is more concise and easier to maintain; update the transition on
.badge (and related selectors if present) accordingly.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (16)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded-element-examples-chromium-light-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-dark-linux.pngis excluded by!**/*.pngplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-element-examples-chromium-light-linux.pngis excluded by!**/*.png
📒 Files selected for processing (15)
playwright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yamlprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scssprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical.component.scssprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
🧰 Additional context used
🧠 Learnings (25)
📚 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/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed.yamlplaywright/snapshots/navbar-vertical-legacy.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical-legacy--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--collapsed-flyout.yamlplaywright/snapshots/navbar-vertical.spec.ts-snapshots/si-navbar-vertical--si-navbar-vertical--mobile-expanded.yaml
📚 Learning: 2025-12-22T13:04:35.578Z
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:35.578Z
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:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scssprojects/element-ng/navbar-vertical/si-navbar-vertical.component.scss
📚 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:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2026-01-07T08:57:45.608Z
Learnt from: dauriamarco
Repo: siemens/element PR: 1226
File: src/app/examples/si-navbar-vertical/si-navbar-vertical.ts:112-115
Timestamp: 2026-01-07T08:57:45.608Z
Learning: Badge color consistency in example files (src/app/examples/) is not a priority and should not be strictly enforced during reviews.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
📚 Learning: 2025-12-22T13:16:19.243Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: projects/element-ng/icon/element-icons.ts:166-167
Timestamp: 2025-12-22T13:16:19.243Z
Learning: In projects/element-ng/icon/element-icons.ts, the elementCheckedImageShape icon intentionally uses base64 encoding instead of utf8 encoding (which is used for other icons). This approach is considered safer and correct for this specific icon and should not be flagged as an inconsistency.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.htmlprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2025-12-09T14:32:34.036Z
Learnt from: Killusions
Repo: siemens/element PR: 998
File: projects/element-ng/chat-messages/si-ai-chat-container.component.html:21-47
Timestamp: 2025-12-09T14:32:34.036Z
Learning: In projects/element-ng/chat-messages, nested if blocks are preferred over switch statements for branching on message types in Angular templates.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
📚 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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.ts
📚 Learning: 2025-12-08T11:25:20.861Z
Learnt from: chintankavathia
Repo: siemens/element PR: 1163
File: projects/element-ng/select/si-select.component.spec.ts:204-205
Timestamp: 2025-12-08T11:25:20.861Z
Learning: In zoneless Angular tests (using provideZonelessChangeDetection), calling fixture.changeDetectorRef.markForCheck() before fixture.detectChanges() is required even for components using the default change detection strategy (not OnPush). This differs from zone.js-based testing where markForCheck is primarily needed only for OnPush components.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.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/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.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/navbar-vertical/si-navbar-vertical-item.component.spec.tsprojects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts
📚 Learning: 2026-01-02T06:41:22.404Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: playwright/e2e/element-examples/static.spec.ts:119-121
Timestamp: 2026-01-02T06:41:22.404Z
Learning: In the Element design system (siemens/element repository), disabling the `aria-required-children` accessibility rule for `SiEmptyStateComponent` is an accepted pattern. This component is used in AG Grid overlays (AgNoRowsOverlayComponent) and other places in Element, and the rule is consistently disabled in tests where this component appears (e.g., si-pills-input, datatable-row-group, ag-grid-empty-state).
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical.component.tsprojects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 Learning: 2025-12-31T07:16:13.451Z
Learnt from: mistrykaran91
Repo: siemens/element PR: 1234
File: projects/element-ng/ag-grid/parts/radio-style.part.ts:15-61
Timestamp: 2025-12-31T07:16:13.451Z
Learning: In the AG Grid theme package (projects/element-ng/ag-grid), styling should maintain consistency with the base Element design system components rather than adding enhancements or extra CSS. If accessibility improvements like focus-visible indicators are needed, they should be implemented at the Element design system level first.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
📚 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-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
📚 Learning: 2026-01-07T08:57:45.608Z
Learnt from: dauriamarco
Repo: siemens/element PR: 1226
File: src/app/examples/si-navbar-vertical/si-navbar-vertical.ts:112-115
Timestamp: 2026-01-07T08:57:45.608Z
Learning: Do not strictly enforce badge color consistency in example files under src/app/examples. Treat badge color checks as non-blocking for TypeScript files in this directory; reviewers may still note inconsistencies, but they should not require changes for these example files. Apply this guidance broadly to all files within src/app/examples.
Applied to files:
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
📚 Learning: 2026-01-09T08:44:09.549Z
Learnt from: spliffone
Repo: siemens/element PR: 896
File: src/app/examples/text/text-word-break.ts:8-8
Timestamp: 2026-01-09T08:44:09.549Z
Learning: In the siemens/element repository, all example components under src/app/examples use the same selector 'app-sample' to denote live-previewer playgrounds. Treat this as a project-wide convention and do not flag such occurrences as duplicate selector issues. If reviewing files outside this directory, apply the same pattern only when the path matches src/app/examples.
Applied to files:
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.tssrc/app/examples/si-navbar-vertical/si-navbar-vertical.ts
📚 Learning: 2025-12-17T14:01:05.705Z
Learnt from: dauriamarco
Repo: siemens/element PR: 908
File: projects/element-ng/side-panel/si-side-panel.component.ts:70-74
Timestamp: 2025-12-17T14:01:05.705Z
Learning: In the Element side-panel component (projects/element-ng/side-panel/si-side-panel.component.ts), the `xxxlMinimum` breakpoint (1920px) is intentionally hardcoded as a private static constant rather than being part of the configurable `Breakpoints` interface. This design preserves full Bootstrap compatibility (which only defines breakpoints up to xxl/1400px) while allowing Element to handle ultra-wide screens explicitly without affecting the standard breakpoint configuration.
Applied to files:
projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss
🧬 Code graph analysis (1)
src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (2)
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (1)
Component(15-133)src/app/examples/si-navbar-vertical/si-navbar-vertical-text.ts (1)
Component(20-60)
⏰ 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 (22)
projects/element-ng/navbar-vertical/si-navbar-vertical.component.scss (1)
135-136: No changes needed. Thedisplay: flexchange is appropriate for this icon button layout.The button contains a single
<si-icon>child element and needs to fill the full width of the collapsed navigation (inline-size: 100%). Usingdisplay: flexis the correct approach here, as it naturally centers the icon within the button without requiring additional workarounds. Changing back todisplay: blockwould require alternative centering strategies (e.g.,text-align: centerwith potential alignment issues for the icon element).projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.ts (2)
16-16: Good addition of text-truncate class.Prevents header text overflow when the navbar is in expanded state, ensuring a clean UI.
35-49: Animation timing refinements look good.The changes improve transition smoothness:
- Header leave animation extended to 0.25s for smoother fade-out
- Header enter delay reduced from 0.3s to 0.1s for snappier appearance
- New divider fade-in at 0.25s coordinates with header fade-out
- Changed to
ease-outfor more natural decelerationThese refinements align well with the PR objectives for animation improvements.
projects/element-ng/navbar-vertical/si-navbar-vertical.component.ts (3)
116-125: LGTM! Well-coordinated expand transition animations.The new animations for item content during expansion:
- Icon repositioning (absolute → relative) over 0.2s ensures proper layout flow
- Title fade-in with 0.1s delay (after icon repositions) creates smooth sequential appearance
- Both animations coordinate properly with the existing 0.5s ease transition
The
optional: trueflags appropriately handle cases where these elements may not exist.
147-156: LGTM! Collapse transition animations properly orchestrated.The collapse animations effectively handle:
- Title fade-out over 0.2s before layout changes
- Badge-collapsed fade-in at 0.5s when dropdown is shown, coordinating with the main collapse timing
Both animations use appropriate timing and the
optional: trueflags ensure robustness when elements are absent.
117-118: All animation selectors are valid and match the DOM structure.The selectors
.item-content .icon,.item-title, and.badge-collapsedare all present insi-navbar-vertical-item.component.html. The.badge-collapsedclass is dynamically applied based on the navbar's collapsed state, ensuring the animations correctly target the updated DOM structure.projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.ts (1)
68-74: LGTM! Badge type detection logic is correct.The
textOnlyBadgecomputed property correctly identifies non-numeric badges for conditional styling. The logic properly handles edge cases:
- Numeric badges (including zero) return
false(not text-only)- String badges return
true(text-only)- Null/undefined badges return
falseThis aligns well with the PR's goal to optimize badge visibility based on badge type and navbar state.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.spec.ts (4)
30-37: LGTM! Good test setup hygiene.Adding
textOnlysignal to the mock and resetting both signals and the spy inbeforeEachensures proper test isolation. This prevents state leakage between tests.
160-178: LGTM! Collapsed state badge behavior is well tested.The test correctly verifies that:
- The
badge-collapsedclass is applied when the navbar is collapsed.- The badge content is formatted consistently (
+99for values > 99).
181-214: LGTM! TextOnly mode tests provide good coverage.The two tests properly verify the conditional badge rendering:
- Badge is hidden when
textOnlyis true.- Badge is visible with correct content when
textOnlyis false.
216-242: LGTM! Comprehensive badge formatting test cases.The test cases cover a good range of scenarios including edge cases for numeric values, the overflow threshold (99/100), and string badges displayed as-is.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss (4)
19-21: LGTM! Wrapper structure supports content overflow control.The
overflow: hiddenon.item-contentprevents content overflow during collapse/expand animations.
44-55: LGTM! Active state color update aligns with PR requirements.The color and indicator bar now use
element-action-primary-hoveras requested in the review comments.
177-180: LGTM! Text-only badge implementation matches PR requirements.Setting
color: transparentandfont-size: 0effectively hides the text while preserving the badge dot shape, as requested for text-only badges in collapsed state.
97-104: The attribute selector correctly targets all emphasis badge variants.The selector
[class*='-emphasis']works as intended. All emphasis badge variants (info-emphasis,success-emphasis,warning-emphasis,danger-emphasis,critical-emphasis,caution-emphasis) consistently follow the-emphasisnaming convention, while non-emphasis variants do not contain this substring. The dynamic class generation ('bg-' + type()) in the badge component ensures this pattern is maintained for all badge types, making the selector a reliable way to identify and keep emphasis badges visible in the collapsed state.src/app/examples/si-navbar-vertical/si-navbar-vertical-routing.ts (3)
24-28: LGTM!The
MenuItemComponentfollows the established pattern for dummy routing components in this example file.
71-74: LGTM!The route entry correctly maps the 'menu-item' path to
MenuItemComponent.
119-136: LGTM! Example demonstrates badge variants effectively.The updated menu items showcase:
- Numeric badges with emphasis colors (
danger-emphasis)- Text badges with emphasis colors (
'Hot','danger-emphasis')- Items without badges
This aligns well with the PR's goal of demonstrating badge visibility behavior. Based on learnings, badge color consistency is non-blocking for example files.
src/app/examples/si-navbar-vertical/si-navbar-vertical.ts (4)
35-45: LGTM! Good variety of badge examples.The Home item with a
'default'badge color and the new Menu item without a badge provide useful contrast for demonstrating badge behavior.
52-75: LGTM! Group badges with children demonstrate aggregation pattern.The User management group with
badge: 6andbadgeColor: 'danger-emphasis'alongside child items with their own badges effectively demonstrates the hierarchical badge pattern.
112-115: Good simplification of the action handler.The action callback no longer mutates the
item.activestate inline. This cleaner approach removes side effects and aligns with the comment warning againsttype: 'action'usage.
116-123: LGTM! Demonstrates numeric badge overflow.The Energy item with
badge: 100will display as+99in the collapsed state, which is a useful example of the badge formatting behavior.
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
Outdated
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
Outdated
Show resolved
Hide resolved
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
43d962c to
b2a0d48
Compare
| /** | ||
| * Keep subtle badges visible when navbar is collapsed. | ||
| */ | ||
| showSubtleBadgeCollapsed?: boolean; |
There was a problem hiding this comment.
Based on the discussion in the PR I though consumers should not be able to configure this?
There was a problem hiding this comment.
Hey @spike-rabbit were you able to clarify this with @panch1739?
There was a problem hiding this comment.
@spike-rabbit I think @dauriamarco is correct, we dont want to enforce that.
spike-rabbit
left a comment
spike-rabbit
left a comment
There was a problem hiding this comment.
sorry, wrong box. Please adopt PR as requested.
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
b2a0d48 to
000d303
Compare
projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.scss
Outdated
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.scss
Outdated
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.html
Outdated
Show resolved
Hide resolved
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
000d303 to
5bf2eec
Compare
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
Outdated
Show resolved
Hide resolved
|
@spike-rabbit @dauriamarco I don't like the number badge behaviour with the approach we simply show a number without any additional information in the accessibility tree. Can we do something to improve the information in the tree? |
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
5bf2eec to
7ac4410
Compare
projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.scss
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-header.component.scss
Outdated
Show resolved
Hide resolved
projects/element-ng/navbar-vertical/si-navbar-vertical-item.component.scss
Show resolved
Hide resolved
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
3 times, most recently
from
46e9e19 to
d67f640
Compare
I see that the conversation continued in #1370, I'll reference it here. |
dauriamarco
added
the
breaking-changes
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
d67f640 to
3f92169
Compare
spike-rabbit
left a comment
spike-rabbit
left a comment
There was a problem hiding this comment.
After separate discussions, we came to the following outcome regarding badge visibility:
By default, the badge should always be visible (expanded and collapsed navbar).
We add a new option called, hideBadgeWhenCollapsed (or a better name if you have other ideas) to hide it when the navbar is collapsed.
dauriamarco
force-pushed
the
refactor/align-vertical-navigation
branch
from
3f92169 to
77c206a
Compare
|
|
dauriamarco
removed
the
breaking-changes
Aligns vertical navigation between iX/Element: badges visibility and behavior improvements.
Refactor: Optimize Notifications/Badges Visibility Logic in Vertical Navigation
Aligns vertical navigation between iX and Element by improving badge visibility/behavior, selected-item visuals, animations, examples, tests, and snapshots.
Key changes
Reviewer checks
No public library API signature changes besides example component/route and example data updates. Estimated review effort: Medium.