feat(navbar-vertical-next): show active item in inline-collapse state#2095
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements an 'inline collapse' feature for the vertical navbar, allowing the active top-level navigation item to be displayed as a chip alongside the toggle button when the navbar is collapsed. The implementation refactors the item's active state into a computed signal and utilizes Angular CDK Portals to relocate the label and icon. Feedback identifies a functional issue where using DomPortal permanently removes the label from its original location, requiring a restoration mechanism to ensure it reappears when the navbar is expanded. Additionally, recommendations were made to include SSR guards for browser-dependent features and to enhance unit tests to verify label visibility after a collapse/expand cycle.
| * template to relocate the label into the inline-collapse chip button. | ||
| * @internal | ||
| */ | ||
| protected readonly labelPortalRef = computed(() => new DomPortal(this.labelContentEl())); |
There was a problem hiding this comment.
Using DomPortal to move the label element into the inline-collapse chip will cause the label to be permanently removed from its original location in the component's template. When the navbar is expanded and the portal is detached, the DOM element is not automatically restored to its original parent. This results in the item label disappearing from the main navigation view after it has been shown in the collapsed state. Consider implementing a mechanism to restore the element to its original parent when the portal is detached. Additionally, ensure that browser-dependent features like CDK Portals are only executed in a browser environment using isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).
References
- When using browser-dependent UI features like Angular CDK Overlay, ensure they are only executed in a browser environment. Use a check like isPlatformBrowser to prevent errors during Server-Side Rendering (SSR).
| it('should not render the chip when the navbar is expanded', async () => { | ||
| await TestBed.inject(Router).navigate(['/item-1/sub-item-1']); | ||
| await fixture.whenStable(); | ||
|
|
||
| component.collapsed.set(false); | ||
| await fixture.whenStable(); | ||
|
|
||
| const host = fixture.nativeElement.querySelector('si-navbar-vertical-next') as HTMLElement; | ||
| expect(host.querySelector(chipSelector)).toBeNull(); | ||
| expect(host.querySelector('.inline-collapse-bar')).toBeNull(); | ||
| }); |
There was a problem hiding this comment.
The current tests verify that the chip is not rendered when the navbar is expanded, but they do not verify that the original label is correctly restored in the main navigation. Given the use of DomPortal to move the label element, it is important to ensure that the label remains visible in the navbar after a collapse/expand cycle. I recommend adding an assertion to check for the label's presence in the expanded navbar. When doing so, prefer checking for at least one instance rather than an exact count to make the test more resilient to future changes.
References
- When testing for the presence of an element, prefer checking for at least one instance rather than an exact count to make the test more resilient to future changes, such as adding more of the same element.
31ad996 to
15d11bb
Compare
5ad5c17 to
c233bdc
Compare
…tive item support
d87394a to
60edab0
Compare
inline-collapsetoggle so users retain context when the navbar is collapsed.Related #1945
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: