feat(navbar-vertical-next): open items in overlay menu when chip is clicked#2282
feat(navbar-vertical-next): open items in overlay menu when chip is clicked#2282mistrykaran91 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the vertical navigation component to improve the collapsed chip menu behavior by using a dedicated CDK overlay and portaling the item labels and container. The review feedback advises replacing manual state propagation via effect() with a declarative linkedSignal for managing the chip menu's open state, which also allows removing an obsolete auto-close effect.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
cd7f3c4 to
66f557a
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces the SiNavbarVerticalNextChipMenuComponent to handle the chip button and overlay menu when the vertical navbar is in inline-collapse mode, replacing the previous DOM portal approach. Additionally, the SiNavbarVerticalNextGroupTriggerDirective was refactored to lazily create its overlay, ensuring correct DOM and accessibility tree ordering. Feedback on the changes focuses on adhering to the repository style guide by replacing manual state propagation via effect() with Angular's linkedSignal for managing the chip menu's open state.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request introduces the SiNavbarVerticalNextChipMenuComponent to manage the chip button and menu overlay in the vertical navbar's inline-collapse mode, refactoring related styles, layout constants, and group trigger logic while adding comprehensive tests. Feedback on the changes suggests replacing an effect() in SiNavbarVerticalNextItemComponent with a computed() signal to adhere to the style guide and simplify the component by removing its constructor. Additionally, it is recommended to call this.detachFlyout() on destruction and wrap browser-dependent CDK overlay operations in an isPlatformBrowser check within SiNavbarVerticalNextGroupTriggerDirective to prevent memory leaks and SSR issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| this.destroyRef.onDestroy(() => { | ||
| this.overlayRef.detach(); | ||
| this.overlayRef.dispose(); | ||
| this.overlayRef?.detach(); | ||
| this.overlayRef?.dispose(); | ||
| }); |
There was a problem hiding this comment.
To prevent potential memory leaks and ensure proper cleanup of the flyout anchor component and outside click subscription, we should call this.detachFlyout() inside the onDestroy hook. Additionally, since Angular CDK Overlay is a browser-dependent UI feature, ensure these operations are only executed in a browser environment using isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).
if (isPlatformBrowser(this.platformId)) {
this.destroyRef.onDestroy(() => {
this.detachFlyout();
this.overlayRef?.detach();
this.overlayRef?.dispose();
});
}References
- When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like
isPlatformBrowserto prevent errors during Server-Side Rendering (SSR).
…licked - The chip previously portalled the active root item into the chip slot and triggered it directly. It is now a button that opens an overlay menu hosting the full items container, so users can navigate to any entry while the sidebar stays parked. - Added `SiNavbarVerticalNextChipMenuComponent` to handle the chip button and menu overlay.
3a95a75 to
e210f17
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the SiNavbarVerticalNextChipMenuComponent to support a chip button and menu overlay for the vertical navbar in inline-collapse mode, utilizing CDK portals to relocate the items container. It also refactors the group trigger directive to lazily create its overlay for correct accessibility tree ordering, updates SCSS layouts, and adds comprehensive unit and E2E tests. A critical issue was identified in SiNavbarVerticalNextItemComponent where using viewChild.required for itemTitleEl can cause a runtime crash (NG0951) during initial change detection; it is recommended to use an optional viewChild and handle the undefined state gracefully.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| private readonly itemTitleEl = viewChild.required<ElementRef<HTMLElement>>('itemTitleEl'); | ||
|
|
||
| /** Signal mirror of the navbar's chip mode. `true` when inline-collapse is active and the navbar is collapsed. | ||
| /** | ||
| * Text label of this item, derived from the projected `<ng-content>`. | ||
| * Consumed by the chip button to mirror the active item's label without | ||
| * duplicating markup. | ||
| * @internal | ||
| */ | ||
| readonly chipMode = computed(() => this.navbar.chipMode()); | ||
| readonly label = computed(() => this.itemTitleEl().nativeElement.textContent?.trim() ?? ''); |
There was a problem hiding this comment.
Using viewChild.required for itemTitleEl can cause a runtime crash (NG0951: Child query [required view child] is not yet available.) during the initial change detection. This happens because si-navbar-vertical-next-chip-menu is declared before <ng-content> in the parent template, meaning the active item's view is not yet initialized when the chip menu evaluates item.label().
To prevent this, use an optional viewChild and handle the undefined state gracefully in the computed signal. Once the view initializes, the signal will automatically update with the correct text. Additionally, ensure that you trim the textContent to remove formatting whitespace.
| private readonly itemTitleEl = viewChild.required<ElementRef<HTMLElement>>('itemTitleEl'); | |
| /** Signal mirror of the navbar's chip mode. `true` when inline-collapse is active and the navbar is collapsed. | |
| /** | |
| * Text label of this item, derived from the projected `<ng-content>`. | |
| * Consumed by the chip button to mirror the active item's label without | |
| * duplicating markup. | |
| * @internal | |
| */ | |
| readonly chipMode = computed(() => this.navbar.chipMode()); | |
| readonly label = computed(() => this.itemTitleEl().nativeElement.textContent?.trim() ?? ''); | |
| private readonly itemTitleEl = viewChild<ElementRef<HTMLElement>>('itemTitleEl'); | |
| /** | |
| * Text label of this item, derived from the projected <ng-content>. | |
| * Consumed by the chip button to mirror the active item's label without | |
| * duplicating markup. | |
| * @internal | |
| */ | |
| readonly label = computed(() => this.itemTitleEl()?.nativeElement.textContent?.trim() ?? ''); |
References
- When reading text content from an ElementRef, trim the textContent to remove formatting whitespace.
panch1739
left a comment
There was a problem hiding this comment.
@mistrykaran91 Awesome!
I think there is a small delay for the Section title...it loads later than the content
Screen.Recording.2026-07-09.at.09.55.44.mov
And another small thing, unrelated to the menu...there is a small bug in Safari. It seems that the expand/collapsed is attached to the selected item instead of having a small gap:
Example safari:
Example Chrome
Related to delay, I'm reusing the same animation which we have for when have an expanded view. I'll take a look into the other issue, related to gap in safari |
SiNavbarVerticalNextChipMenuComponentto handle the chip button and menu overlay.Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: