updated send-filters-nav - address feedback

This commit is contained in:
Isaac Ivins
2025-12-05 13:35:33 -05:00
parent d03f84c153
commit 8d5a246fbf
5 changed files with 38 additions and 46 deletions

View File

@@ -1,4 +1,4 @@
import { Component } from "@angular/core";
import { ChangeDetectionStrategy, Component } from "@angular/core";
import { ComponentFixture, TestBed } from "@angular/core/testing";
import { RouterModule } from "@angular/router";
import { mock } from "jest-mock-extended";
@@ -9,10 +9,10 @@ import { NavigationModule } from "@bitwarden/components";
import { DesktopLayoutComponent } from "./desktop-layout.component";
// Mock the child component to isolate DesktopLayoutComponent testing
// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection
@Component({
selector: "app-send-filters-nav",
template: "",
changeDetection: ChangeDetectionStrategy.OnPush,
})
class MockSendFiltersNavComponent {}

View File

@@ -8,12 +8,12 @@
icon="bwi-file-text"
[text]="'sendTypeText' | i18n"
(click)="selectTypeAndNavigate(SendType.Text); $event.stopPropagation()"
[forceActiveStyles]="isTypeActive(SendType.Text)"
[forceActiveStyles]="activeSendType() === SendType.Text"
></bit-nav-item>
<bit-nav-item
icon="bwi-file"
[text]="'sendTypeFile' | i18n"
(click)="selectTypeAndNavigate(SendType.File); $event.stopPropagation()"
[forceActiveStyles]="isTypeActive(SendType.File)"
[forceActiveStyles]="activeSendType() === SendType.File"
></bit-nav-item>
</bit-nav-group>

View File

@@ -1,4 +1,4 @@
import { Component } from "@angular/core";
import { ChangeDetectionStrategy, Component } from "@angular/core";
import { ComponentFixture, TestBed } from "@angular/core/testing";
import { Router, provideRouter } from "@angular/router";
import { RouterTestingHarness } from "@angular/router/testing";
@@ -11,9 +11,7 @@ import { SendListFiltersService } from "@bitwarden/send-ui";
import { SendFiltersNavComponent } from "./send-filters-nav.component";
// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush
// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection
@Component({ template: "" })
@Component({ template: "", changeDetection: ChangeDetectionStrategy.OnPush })
class DummyComponent {}
Object.defineProperty(window, "matchMedia", {
@@ -54,6 +52,7 @@ describe("SendFiltersNavComponent", () => {
filterFormValueSubject.next(mockSendListFiltersService.filterForm.value);
}),
} as any,
filters$: filterFormValueSubject.asObservable(),
};
await TestBed.configureTestingModule({
@@ -104,6 +103,7 @@ describe("SendFiltersNavComponent", () => {
describe("isSendRouteActive", () => {
it("returns true when on /new-sends route", async () => {
await harness.navigateByUrl("/new-sends");
fixture.detectChanges();
expect(component["isSendRouteActive"]()).toBe(true);
});
@@ -113,33 +113,31 @@ describe("SendFiltersNavComponent", () => {
});
});
describe("isTypeActive", () => {
it("returns true when on send route and filter type matches", async () => {
describe("activeSendType", () => {
it("returns the active send type when on send route and filter type is set", async () => {
await harness.navigateByUrl("/new-sends");
mockSendListFiltersService.filterForm.value = { sendType: SendType.Text };
filterFormValueSubject.next({ sendType: SendType.Text });
fixture.detectChanges();
expect(component["isTypeActive"](SendType.Text)).toBe(true);
expect(component["isTypeActive"](SendType.File)).toBe(false);
expect(component["activeSendType"]()).toBe(SendType.Text);
});
it("returns false when not on send route", () => {
it("returns undefined when not on send route", () => {
mockSendListFiltersService.filterForm.value = { sendType: SendType.Text };
filterFormValueSubject.next({ sendType: SendType.Text });
fixture.detectChanges();
expect(component["isTypeActive"](SendType.Text)).toBe(false);
expect(component["activeSendType"]()).toBeUndefined();
});
it("returns false when no type is selected", async () => {
it("returns null when on send route but no type is selected", async () => {
await harness.navigateByUrl("/new-sends");
mockSendListFiltersService.filterForm.value = { sendType: null };
filterFormValueSubject.next({ sendType: null });
fixture.detectChanges();
expect(component["isTypeActive"](SendType.Text)).toBe(false);
expect(component["isTypeActive"](SendType.File)).toBe(false);
expect(component["activeSendType"]()).toBeNull();
});
});
@@ -180,7 +178,7 @@ describe("SendFiltersNavComponent", () => {
});
});
it("does not navigate when already on send route", async () => {
it("does not navigate when already on send route (component is reactive)", async () => {
await harness.navigateByUrl("/new-sends");
const router = TestBed.inject(Router);
const navigateSpy = jest.spyOn(router, "navigate");

View File

@@ -1,8 +1,8 @@
import { CommonModule } from "@angular/common";
import { Component, inject } from "@angular/core";
import { ChangeDetectionStrategy, Component, computed, inject } from "@angular/core";
import { toSignal } from "@angular/core/rxjs-interop";
import { Router } from "@angular/router";
import { startWith } from "rxjs";
import { NavigationEnd, Router } from "@angular/router";
import { filter, map, startWith } from "rxjs";
import { SendType } from "@bitwarden/common/tools/send/enums/send-type";
import { NavigationModule } from "@bitwarden/components";
@@ -11,50 +11,46 @@ import { I18nPipe } from "@bitwarden/ui-common";
/**
* Navigation component that renders Send filter options in the sidebar.
* Follows reactive pattern: updates filter state, display component reacts.
* Fully reactive using signals - no manual subscriptions or method-based computed values.
* - Parent "Send" nav-group clears filter (shows all sends)
* - Child "Text"/"File" items set filter to specific type
* - Active states computed from current filter value + route
* - Active states computed reactively from filter signal + route signal
*/
// 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: "app-send-filters-nav",
templateUrl: "./send-filters-nav.component.html",
changeDetection: ChangeDetectionStrategy.OnPush,
imports: [CommonModule, NavigationModule, I18nPipe],
})
export class SendFiltersNavComponent {
protected readonly SendType = SendType;
// Inject services at class level
private readonly filtersService = inject(SendListFiltersService);
private readonly router = inject(Router);
// Convert filter form to signal for reactive updates
private readonly currentFilter = toSignal(
this.filtersService.filterForm.valueChanges.pipe(
startWith(this.filtersService.filterForm.value),
// Convert filters$ observable to signal for reactive updates
private readonly currentFilter = toSignal(this.filtersService.filters$);
// Track whether current route is the send route
private readonly isSendRouteActive = toSignal(
this.router.events.pipe(
filter((event) => event instanceof NavigationEnd),
map((event) => (event as NavigationEnd).urlAfterRedirects.includes("/new-sends")),
startWith(this.router.url.includes("/new-sends")),
),
{ initialValue: this.filtersService.filterForm.value },
{ initialValue: this.router.url.includes("/new-sends") },
);
// Computed: Is send route currently active?
private isSendRouteActive(): boolean {
return this.router.url.includes("/new-sends");
}
// Computed: Active send type (null when on send route with no filter, undefined when not on send route)
protected readonly activeSendType = computed(() => {
return this.isSendRouteActive() ? this.currentFilter()?.sendType : undefined;
});
// Computed: Is specific type currently active (on send route AND that filter is set)?
protected isTypeActive(type: SendType): boolean {
return this.isSendRouteActive() && this.currentFilter()?.sendType === type;
}
// Set filter and navigate to send route if needed
// - No type parameter (undefined): clears filter (shows all sends)
// - Specific type: filters to that send type
// Update send filter and navigate to /new-sends (only if not already there - send-v2 component reacts to filter changes)
protected async selectTypeAndNavigate(type?: SendType): Promise<void> {
this.filtersService.filterForm.patchValue({ sendType: type !== undefined ? type : null });
if (!this.isSendRouteActive()) {
if (!this.router.url.includes("/new-sends")) {
await this.router.navigate(["/new-sends"]);
}
}

View File

@@ -98,7 +98,6 @@ export class SendV2Component extends BaseSendComponent implements OnInit, OnDest
this.searchBarService.searchText$.pipe(takeUntilDestroyed()).subscribe((searchText) => {
this.searchText = searchText;
this.searchTextChanged();
setTimeout(() => this.cdr.detectChanges(), 250);
});
// Listen to filter changes from sidebar navigation
@@ -106,7 +105,6 @@ export class SendV2Component extends BaseSendComponent implements OnInit, OnDest
.pipe(takeUntilDestroyed())
.subscribe((filters) => {
this.applySendTypeFilter(filters);
this.cdr.detectChanges();
});
}