From d64db8fbf58a90769683c2b00c7cc0e4ac0d6180 Mon Sep 17 00:00:00 2001 From: Oscar Hinton Date: Mon, 26 Jan 2026 17:44:16 +0100 Subject: [PATCH] [CL-904] Migrate CL/Navigation to use OnPush (#16958) * Migrate CL/Navigation to use OnPush * Modernize the code * Swap to signals and class * Further tweaks * Remove this. * Replace setOpen and setClose with a public signal * fix merge issues and signal-ifying service * fix class and style bindings * fix accidental behavior change from merge conflicts * fix redundant check * fix missed ngClass * fix comment * Re-add share ng-template --------- Co-authored-by: Vicki League Co-authored-by: Will Martin Co-authored-by: Claude Sonnet 4.5 --- .../src/layout/layout.component.html | 23 ++-- .../src/navigation/nav-base.component.ts | 20 +-- .../src/navigation/nav-divider.component.html | 2 +- .../src/navigation/nav-divider.component.ts | 12 +- .../src/navigation/nav-group.component.html | 4 +- .../src/navigation/nav-group.component.ts | 42 +++--- .../src/navigation/nav-group.stories.ts | 5 +- .../src/navigation/nav-item.component.html | 69 +++++----- .../src/navigation/nav-item.component.ts | 62 +++++---- .../src/navigation/nav-logo.component.html | 15 +-- .../src/navigation/nav-logo.component.ts | 26 ++-- .../src/navigation/side-nav.component.html | 126 ++++++++---------- .../src/navigation/side-nav.component.ts | 30 +++-- .../src/navigation/side-nav.service.ts | 69 +++------- 14 files changed, 240 insertions(+), 265 deletions(-) diff --git a/libs/components/src/layout/layout.component.html b/libs/components/src/layout/layout.component.html index 66bfcafafe9..f0e2b601e38 100644 --- a/libs/components/src/layout/layout.component.html +++ b/libs/components/src/layout/layout.component.html @@ -30,21 +30,14 @@ - @if ( - { - open: sideNavService.open$ | async, - }; - as data - ) { -
- @if (data.open) { -
- } -
- } +
+ @if (sideNavService.open()) { +
+ } +
diff --git a/libs/components/src/navigation/nav-base.component.ts b/libs/components/src/navigation/nav-base.component.ts index 706df2b25ad..e20edf5a0f9 100644 --- a/libs/components/src/navigation/nav-base.component.ts +++ b/libs/components/src/navigation/nav-base.component.ts @@ -1,8 +1,11 @@ -import { Directive, EventEmitter, Output, input, model } from "@angular/core"; +import { Directive, output, input, model } from "@angular/core"; import { RouterLink, RouterLinkActive } from "@angular/router"; /** - * `NavGroupComponent` builds upon `NavItemComponent`. This class represents the properties that are passed down to `NavItemComponent`. + * Base class for navigation components in the side navigation. + * + * `NavGroupComponent` builds upon `NavItemComponent`. This class represents the properties + * that are passed down to `NavItemComponent`. */ @Directive() export abstract class NavBaseComponent { @@ -38,23 +41,26 @@ export abstract class NavBaseComponent { * * --- * + * @remarks * We can't name this "routerLink" because Angular will mount the `RouterLink` directive. * - * See: {@link https://github.com/angular/angular/issues/24482} + * @see {@link RouterLink.routerLink} + * @see {@link https://github.com/angular/angular/issues/24482} */ readonly route = input(); /** * Passed to internal `routerLink` * - * See {@link RouterLink.relativeTo} + * @see {@link RouterLink.relativeTo} */ readonly relativeTo = input(); /** * Passed to internal `routerLink` * - * See {@link RouterLinkActive.routerLinkActiveOptions} + * @default { paths: "subset", queryParams: "ignored", fragment: "ignored", matrixParams: "ignored" } + * @see {@link RouterLinkActive.routerLinkActiveOptions} */ readonly routerLinkActiveOptions = input({ paths: "subset", @@ -71,7 +77,5 @@ export abstract class NavBaseComponent { /** * Fires when main content is clicked */ - // FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals - // eslint-disable-next-line @angular-eslint/prefer-output-emitter-ref - @Output() mainContentClicked: EventEmitter = new EventEmitter(); + readonly mainContentClicked = output(); } diff --git a/libs/components/src/navigation/nav-divider.component.html b/libs/components/src/navigation/nav-divider.component.html index 2d8e1dfa24b..7af7de2a28a 100644 --- a/libs/components/src/navigation/nav-divider.component.html +++ b/libs/components/src/navigation/nav-divider.component.html @@ -1,3 +1,3 @@ -@if (sideNavService.open$ | async) { +@if (sideNavService.open()) {
} diff --git a/libs/components/src/navigation/nav-divider.component.ts b/libs/components/src/navigation/nav-divider.component.ts index 2f33883fd58..05a69563312 100644 --- a/libs/components/src/navigation/nav-divider.component.ts +++ b/libs/components/src/navigation/nav-divider.component.ts @@ -1,15 +1,15 @@ -import { CommonModule } from "@angular/common"; -import { Component } from "@angular/core"; +import { ChangeDetectionStrategy, Component, inject } from "@angular/core"; import { SideNavService } from "./side-nav.service"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection +/** + * A visual divider for separating navigation items in the side navigation. + */ @Component({ selector: "bit-nav-divider", templateUrl: "./nav-divider.component.html", - imports: [CommonModule], + changeDetection: ChangeDetectionStrategy.OnPush, }) export class NavDividerComponent { - constructor(protected sideNavService: SideNavService) {} + protected readonly sideNavService = inject(SideNavService); } diff --git a/libs/components/src/navigation/nav-group.component.html b/libs/components/src/navigation/nav-group.component.html index d305f89063e..26d1c68da43 100644 --- a/libs/components/src/navigation/nav-group.component.html +++ b/libs/components/src/navigation/nav-group.component.html @@ -20,9 +20,7 @@ - + } @if (open) {
}
+ + + +
+ @if (icon()) { + + } + @if (open) { + {{ text() }} + } +
+ diff --git a/libs/components/src/navigation/nav-item.component.ts b/libs/components/src/navigation/nav-item.component.ts index e57413d9980..53b181ec083 100644 --- a/libs/components/src/navigation/nav-item.component.ts +++ b/libs/components/src/navigation/nav-item.component.ts @@ -1,7 +1,14 @@ -import { CommonModule } from "@angular/common"; -import { Component, HostListener, Optional, computed, input, model } from "@angular/core"; -import { RouterLinkActive, RouterModule } from "@angular/router"; -import { BehaviorSubject, map } from "rxjs"; +import { NgTemplateOutlet } from "@angular/common"; +import { + ChangeDetectionStrategy, + Component, + input, + inject, + signal, + computed, + model, +} from "@angular/core"; +import { RouterModule, RouterLinkActive } from "@angular/router"; import { IconButtonModule } from "../icon-button"; @@ -14,13 +21,16 @@ export abstract class NavGroupAbstraction { abstract treeDepth: ReturnType>; } -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "bit-nav-item", templateUrl: "./nav-item.component.html", providers: [{ provide: NavBaseComponent, useExisting: NavItemComponent }], - imports: [CommonModule, IconButtonModule, RouterModule], + imports: [NgTemplateOutlet, IconButtonModule, RouterModule], + changeDetection: ChangeDetectionStrategy.OnPush, + host: { + "(focusin)": "onFocusIn($event.target)", + "(focusout)": "onFocusOut()", + }, }) export class NavItemComponent extends NavBaseComponent { /** @@ -35,9 +45,14 @@ export class NavItemComponent extends NavBaseComponent { */ protected readonly TREE_DEPTH_PADDING = 1.75; - /** Forces active styles to be shown, regardless of the `routerLinkActiveOptions` */ + /** + * Forces active styles to be shown, regardless of the `routerLinkActiveOptions` + */ readonly forceActiveStyles = input(false); + protected readonly sideNavService = inject(SideNavService); + private readonly parentNavGroup = inject(NavGroupAbstraction, { optional: true }); + /** * Is `true` if `to` matches the current route */ @@ -56,7 +71,7 @@ export class NavItemComponent extends NavBaseComponent { * adding calculation for tree variant due to needing visual alignment on different indentation levels needed between the first level and subsequent levels */ protected readonly navItemIndentationPadding = computed(() => { - const open = this.sideNavService.open; + const open = this.sideNavService.open(); const depth = this.treeDepth() ?? 0; if (open && this.variant() === "tree") { @@ -87,25 +102,22 @@ export class NavItemComponent extends NavBaseComponent { * (denoted with the data-fvw attribute) matches :focus-visible. We then map that state to some * styles, so the entire component can have an outline. */ - protected focusVisibleWithin$ = new BehaviorSubject(false); - protected fvwStyles$ = this.focusVisibleWithin$.pipe( - map((value) => - value ? "tw-z-10 tw-rounded tw-outline-none tw-ring tw-ring-inset tw-ring-border-focus" : "", - ), + protected readonly focusVisibleWithin = signal(false); + protected readonly fvwStyles = computed(() => + this.focusVisibleWithin() + ? "tw-z-10 tw-rounded tw-outline-none tw-ring tw-ring-inset tw-ring-border-focus" + : "", ); - @HostListener("focusin", ["$event.target"]) - onFocusIn(target: HTMLElement) { - this.focusVisibleWithin$.next(target.matches("[data-fvw]:focus-visible")); - } - @HostListener("focusout") - onFocusOut() { - this.focusVisibleWithin$.next(false); + + protected onFocusIn(target: HTMLElement) { + this.focusVisibleWithin.set(target.matches("[data-fvw]:focus-visible")); } - constructor( - protected sideNavService: SideNavService, - @Optional() private parentNavGroup: NavGroupAbstraction, - ) { + protected onFocusOut() { + this.focusVisibleWithin.set(false); + } + + constructor() { super(); // Set tree depth based on parent's depth diff --git a/libs/components/src/navigation/nav-logo.component.html b/libs/components/src/navigation/nav-logo.component.html index 1d9961554c2..9f18855ae13 100644 --- a/libs/components/src/navigation/nav-logo.component.html +++ b/libs/components/src/navigation/nav-logo.component.html @@ -1,22 +1,21 @@
- +
diff --git a/libs/components/src/navigation/nav-logo.component.ts b/libs/components/src/navigation/nav-logo.component.ts index 0602e8b753c..fec50ee8902 100644 --- a/libs/components/src/navigation/nav-logo.component.ts +++ b/libs/components/src/navigation/nav-logo.component.ts @@ -1,5 +1,4 @@ -import { CommonModule } from "@angular/common"; -import { Component, input } from "@angular/core"; +import { ChangeDetectionStrategy, Component, input, inject } from "@angular/core"; import { RouterLinkActive, RouterLink } from "@angular/router"; import { BitwardenShield, Icon } from "@bitwarden/assets/svg"; @@ -8,18 +7,25 @@ import { BitIconComponent } from "../icon/icon.component"; import { SideNavService } from "./side-nav.service"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ selector: "bit-nav-logo", templateUrl: "./nav-logo.component.html", - imports: [CommonModule, RouterLinkActive, RouterLink, BitIconComponent], + imports: [RouterLinkActive, RouterLink, BitIconComponent], + changeDetection: ChangeDetectionStrategy.OnPush, }) export class NavLogoComponent { - /** Icon that is displayed when the side nav is closed */ + protected readonly sideNavService = inject(SideNavService); + + /** + * Icon that is displayed when the side nav is closed + * + * @default BitwardenShield + */ readonly closedIcon = input(BitwardenShield); - /** Icon that is displayed when the side nav is open */ + /** + * Icon that is displayed when the side nav is open + */ readonly openIcon = input.required(); /** @@ -27,8 +33,8 @@ export class NavLogoComponent { */ readonly route = input.required(); - /** Passed to `attr.aria-label` and `attr.title` */ + /** + * Passed to `attr.aria-label` and `attr.title` + */ readonly label = input.required(); - - constructor(protected sideNavService: SideNavService) {} } diff --git a/libs/components/src/navigation/side-nav.component.html b/libs/components/src/navigation/side-nav.component.html index b70d650622a..78fed07011d 100644 --- a/libs/components/src/navigation/side-nav.component.html +++ b/libs/components/src/navigation/side-nav.component.html @@ -1,68 +1,60 @@ -@if ( - { - open: sideNavService.open$ | async, - isOverlay: sideNavService.isOverlay$ | async, - }; - as data -) { -
- +@let open = sideNavService.open(); +@let isOverlay = sideNavService.isOverlay(); + +
+
-} + class="[@media(min-height:53rem)]:tw-sticky tw-bottom-0 tw-left-0 tw-z-20 tw-mt-auto tw-w-full tw-bg-bg-sidenav" + > + + @if (open) { + + } +
+ +
+
+ + +
diff --git a/libs/components/src/navigation/side-nav.component.ts b/libs/components/src/navigation/side-nav.component.ts index b13920d9749..35835f1be96 100644 --- a/libs/components/src/navigation/side-nav.component.ts +++ b/libs/components/src/navigation/side-nav.component.ts @@ -1,7 +1,14 @@ import { CdkTrapFocus } from "@angular/cdk/a11y"; import { DragDropModule, CdkDragMove } from "@angular/cdk/drag-drop"; -import { CommonModule } from "@angular/common"; -import { Component, ElementRef, inject, input, viewChild } from "@angular/core"; +import { AsyncPipe } from "@angular/common"; +import { + ChangeDetectionStrategy, + Component, + ElementRef, + input, + viewChild, + inject, +} from "@angular/core"; import { I18nPipe } from "@bitwarden/ui-common"; @@ -12,35 +19,42 @@ import { SideNavService } from "./side-nav.service"; export type SideNavVariant = "primary" | "secondary"; -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection +/** + * Side navigation component that provides a collapsible navigation menu. + */ @Component({ selector: "bit-side-nav", templateUrl: "side-nav.component.html", imports: [ - CommonModule, CdkTrapFocus, NavDividerComponent, BitIconButtonComponent, I18nPipe, DragDropModule, + AsyncPipe, ], host: { class: "tw-block tw-h-full", }, + changeDetection: ChangeDetectionStrategy.OnPush, }) export class SideNavComponent { - protected sideNavService = inject(SideNavService); + protected readonly sideNavService = inject(SideNavService); + /** + * Visual variant of the side navigation + * + * @default "primary" + */ readonly variant = input("primary"); private readonly toggleButton = viewChild("toggleButton", { read: ElementRef }); private elementRef = inject>(ElementRef); - protected handleKeyDown = (event: KeyboardEvent) => { + protected readonly handleKeyDown = (event: KeyboardEvent) => { if (event.key === "Escape") { - this.sideNavService.setClose(); + this.sideNavService.open.set(false); this.toggleButton()?.nativeElement.focus(); return false; } diff --git a/libs/components/src/navigation/side-nav.service.ts b/libs/components/src/navigation/side-nav.service.ts index 63e54c81fe5..05713006a43 100644 --- a/libs/components/src/navigation/side-nav.service.ts +++ b/libs/components/src/navigation/side-nav.service.ts @@ -1,15 +1,6 @@ -import { inject, Injectable } from "@angular/core"; -import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { - BehaviorSubject, - Observable, - combineLatest, - fromEvent, - map, - startWith, - debounceTime, - first, -} from "rxjs"; +import { computed, effect, inject, Injectable, signal } from "@angular/core"; +import { takeUntilDestroyed, toSignal } from "@angular/core/rxjs-interop"; +import { BehaviorSubject, Observable, fromEvent, map, startWith, debounceTime, first } from "rxjs"; import { BIT_SIDE_NAV_DISK, GlobalStateProvider, KeyDefinition } from "@bitwarden/state"; @@ -32,16 +23,17 @@ export class SideNavService { private rootFontSizePx: number; - private _open$ = new BehaviorSubject(isAtOrLargerThanBreakpoint("md")); - open$ = this._open$.asObservable(); + /** + * Whether the side navigation is open or closed. + */ + readonly open = signal(isAtOrLargerThanBreakpoint("md")); private isLargeScreen$ = media(`(min-width: ${BREAKPOINTS.md}px)`); - private _userCollapsePreference$ = new BehaviorSubject(null); - userCollapsePreference$ = this._userCollapsePreference$.asObservable(); + readonly isLargeScreen = toSignal(this.isLargeScreen$, { requireSync: true }); - isOverlay$ = combineLatest([this.open$, this.isLargeScreen$]).pipe( - map(([open, isLargeScreen]) => open && !isLargeScreen), - ); + readonly userCollapsePreference = signal(null); + + readonly isOverlay = computed(() => this.open() && !this.isLargeScreen()); /** * Local component state width @@ -67,16 +59,14 @@ export class SideNavService { this.rootFontSizePx = parseFloat(getComputedStyle(document.documentElement).fontSize || "16"); // Handle open/close state - combineLatest([this.isLargeScreen$, this.userCollapsePreference$]) - .pipe(takeUntilDestroyed()) - .subscribe(([isLargeScreen, userCollapsePreference]) => { - if (!isLargeScreen) { - this.setClose(); - } else if (userCollapsePreference !== "closed") { - // Auto-open when user hasn't set preference (null) or prefers open - this.setOpen(); - } - }); + effect(() => { + if (!this.isLargeScreen()) { + this.open.set(false); + } else if (this.userCollapsePreference() !== "closed") { + // Auto-open when user hasn't set preference (null) or prefers open + this.open.set(true); + } + }); // Initialize the resizable width from state provider this.widthState$.pipe(first()).subscribe((width: number) => { @@ -89,31 +79,14 @@ export class SideNavService { }); } - get open() { - return this._open$.getValue(); - } - - setOpen() { - this._open$.next(true); - } - - setClose() { - this._open$.next(false); - } - /** * Toggle the open/close state of the side nav */ toggle() { - const curr = this._open$.getValue(); // Store user's preference based on what state they're toggling TO - this._userCollapsePreference$.next(curr ? "closed" : "open"); + this.userCollapsePreference.set(this.open() ? "closed" : "open"); - if (curr) { - this.setClose(); - } else { - this.setOpen(); - } + this.open.set(!this.open()); } /**