From 60b7e142cae393c10cf7bf8b3d2beb6ed4c5d986 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 7 Nov 2024 12:50:41 -0600 Subject: [PATCH 01/22] move vault headings to their own component --- .../vault/vault-header-v2.component.html | 2 + .../vault/vault-header-v2.component.spec.ts | 65 +++++++++++++++++++ .../vault/vault-header-v2.component.ts | 12 ++++ .../components/vault/vault-v2.component.html | 3 +- .../components/vault/vault-v2.component.ts | 7 +- 5 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 apps/browser/src/vault/popup/components/vault/vault-header-v2.component.html create mode 100644 apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts create mode 100644 apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts diff --git a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.html new file mode 100644 index 00000000000..149a51629df --- /dev/null +++ b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.html @@ -0,0 +1,2 @@ + + diff --git a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts new file mode 100644 index 00000000000..f498f6af0f5 --- /dev/null +++ b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts @@ -0,0 +1,65 @@ +import { CommonModule } from "@angular/common"; +import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { ActivatedRoute } from "@angular/router"; +import { mock } from "jest-mock-extended"; +import { BehaviorSubject, Subject } from "rxjs"; + +import { CollectionService } from "@bitwarden/admin-console/common"; +import { SearchService } from "@bitwarden/common/abstractions/search.service"; +import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; +import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; +import { MessageSender } from "@bitwarden/common/platform/messaging"; +import { SyncService } from "@bitwarden/common/platform/sync"; +import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; +import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; +import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; +import { PasswordRepromptService } from "@bitwarden/vault"; + +import { AutofillService } from "../../../../autofill/services/abstractions/autofill.service"; + +import { VaultHeaderV2Component } from "./vault-header-v2.component"; + +describe("VaultHeaderV2Component", () => { + let component: VaultHeaderV2Component; + let fixture: ComponentFixture; + + beforeEach(async () => { + await TestBed.configureTestingModule({ + imports: [VaultHeaderV2Component, CommonModule], + providers: [ + { + provide: CipherService, + useValue: mock({ cipherViews$: new BehaviorSubject([]) }), + }, + { provide: VaultSettingsService, useValue: mock() }, + { provide: FolderService, useValue: mock() }, + { provide: OrganizationService, useValue: mock() }, + { provide: CollectionService, useValue: mock() }, + { provide: PolicyService, useValue: mock() }, + { provide: SearchService, useValue: mock() }, + { provide: PlatformUtilsService, useValue: mock() }, + { provide: AutofillService, useValue: mock() }, + { provide: PasswordRepromptService, useValue: mock() }, + { provide: MessageSender, useValue: mock() }, + { provide: AccountService, useValue: mock() }, + { + provide: SyncService, + useValue: mock({ activeUserLastSync$: () => new Subject() }), + }, + { provide: ActivatedRoute, useValue: { queryParams: new BehaviorSubject({}) } }, + { provide: I18nService, useValue: { t: (key: string) => key } }, + ], + }).compileComponents(); + + fixture = TestBed.createComponent(VaultHeaderV2Component); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it("creates", () => { + expect(component).toBeTruthy(); + }); +}); diff --git a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts new file mode 100644 index 00000000000..22743a4e206 --- /dev/null +++ b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts @@ -0,0 +1,12 @@ +import { Component } from "@angular/core"; + +import { VaultListFiltersComponent } from "../vault-v2/vault-list-filters/vault-list-filters.component"; +import { VaultV2SearchComponent } from "../vault-v2/vault-search/vault-v2-search.component"; + +@Component({ + selector: "app-vault-header-v2", + templateUrl: "vault-header-v2.component.html", + standalone: true, + imports: [VaultV2SearchComponent, VaultListFiltersComponent], +}) +export class VaultHeaderV2Component {} diff --git a/apps/browser/src/vault/popup/components/vault/vault-v2.component.html b/apps/browser/src/vault/popup/components/vault/vault-v2.component.html index e402e131436..be9d959f993 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault/vault-v2.component.html @@ -24,8 +24,7 @@
- - +
diff --git a/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts b/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts index 21c71332997..ae0a61163cd 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts @@ -23,8 +23,8 @@ import { NewItemDropdownV2Component, NewItemInitialValues, } from "../vault-v2/new-item-dropdown/new-item-dropdown-v2.component"; -import { VaultListFiltersComponent } from "../vault-v2/vault-list-filters/vault-list-filters.component"; -import { VaultV2SearchComponent } from "../vault-v2/vault-search/vault-v2-search.component"; + +import { VaultHeaderV2Component } from "./vault-header-v2.component"; enum VaultState { Empty, @@ -46,12 +46,11 @@ enum VaultState { CommonModule, AutofillVaultListItemsComponent, VaultListItemsContainerComponent, - VaultListFiltersComponent, ButtonModule, RouterLink, - VaultV2SearchComponent, NewItemDropdownV2Component, ScrollingModule, + VaultHeaderV2Component, ], providers: [VaultUiOnboardingService], }) From 82ae9de67081a9399009dcbf1f6f7ec92b6e8d93 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 7 Nov 2024 12:53:37 -0600 Subject: [PATCH 02/22] update aria-label to bind to the data attribute --- .../vault-list-filters/vault-list-filters.component.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html index d9c4fbeee15..56f35c41f6d 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-list-filters/vault-list-filters.component.html @@ -1,4 +1,4 @@ -
+
Date: Thu, 7 Nov 2024 13:04:17 -0600 Subject: [PATCH 03/22] move vault headings to the vault-v2 folder --- .../vault-header}/vault-header-v2.component.html | 0 .../vault-header}/vault-header-v2.component.spec.ts | 2 +- .../vault-header/vault-header-v2.component.ts | 12 ++++++++++++ .../popup/components/vault/vault-v2.component.ts | 3 +-- 4 files changed, 14 insertions(+), 3 deletions(-) rename apps/browser/src/vault/popup/components/{vault => vault-v2/vault-header}/vault-header-v2.component.html (100%) rename apps/browser/src/vault/popup/components/{vault => vault-v2/vault-header}/vault-header-v2.component.spec.ts (97%) create mode 100644 apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts diff --git a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html similarity index 100% rename from apps/browser/src/vault/popup/components/vault/vault-header-v2.component.html rename to apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html diff --git a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts similarity index 97% rename from apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts rename to apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index f498f6af0f5..26285460049 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -18,7 +18,7 @@ import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folde import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; import { PasswordRepromptService } from "@bitwarden/vault"; -import { AutofillService } from "../../../../autofill/services/abstractions/autofill.service"; +import { AutofillService } from "../../../../../autofill/services/abstractions/autofill.service"; import { VaultHeaderV2Component } from "./vault-header-v2.component"; diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts new file mode 100644 index 00000000000..029c157bd18 --- /dev/null +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -0,0 +1,12 @@ +import { Component } from "@angular/core"; + +import { VaultListFiltersComponent } from "../vault-list-filters/vault-list-filters.component"; +import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.component"; + +@Component({ + selector: "app-vault-header-v2", + templateUrl: "vault-header-v2.component.html", + standalone: true, + imports: [VaultV2SearchComponent, VaultListFiltersComponent], +}) +export class VaultHeaderV2Component {} diff --git a/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts b/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts index ae0a61163cd..68aa40cbf62 100644 --- a/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault/vault-v2.component.ts @@ -23,8 +23,7 @@ import { NewItemDropdownV2Component, NewItemInitialValues, } from "../vault-v2/new-item-dropdown/new-item-dropdown-v2.component"; - -import { VaultHeaderV2Component } from "./vault-header-v2.component"; +import { VaultHeaderV2Component } from "../vault-v2/vault-header/vault-header-v2.component"; enum VaultState { Empty, From b2c50ffc4ef42ecffdaf56b2e9509230794440c0 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 7 Nov 2024 13:53:51 -0600 Subject: [PATCH 04/22] integrate disclosure trigger to hide vault filters --- .../vault-header/vault-header-v2.component.html | 16 ++++++++++++++-- .../vault-header/vault-header-v2.component.ts | 13 ++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 149a51629df..ae626573050 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -1,2 +1,14 @@ - - +
+
+ +
+ +
+ + + diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 029c157bd18..7f47db4c291 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,5 +1,9 @@ +import { NgClass } from "@angular/common"; import { Component } from "@angular/core"; +import { DisclosureTriggerForDirective, IconButtonModule } from "@bitwarden/components"; + +import { DisclosureComponent } from "../../../../../../../../libs/components/src/disclosure/disclosure.component"; import { VaultListFiltersComponent } from "../vault-list-filters/vault-list-filters.component"; import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.component"; @@ -7,6 +11,13 @@ import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.componen selector: "app-vault-header-v2", templateUrl: "vault-header-v2.component.html", standalone: true, - imports: [VaultV2SearchComponent, VaultListFiltersComponent], + imports: [ + VaultV2SearchComponent, + VaultListFiltersComponent, + DisclosureComponent, + IconButtonModule, + DisclosureTriggerForDirective, + NgClass, + ], }) export class VaultHeaderV2Component {} From 39b2e25df7144ac1341411450b04fe2bfe480ace Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 7 Nov 2024 13:54:27 -0600 Subject: [PATCH 05/22] remove built in margin on search component - spacing will be managed by the parent component --- .../vault-search/vault-v2-search.component.html | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.html index 55674aa83e5..f17d668d57b 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-search/vault-v2-search.component.html @@ -1,8 +1,6 @@ -
- - -
+ + From 4b062b0483a804089ce0afd369a91fe55c9cc954 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Fri, 8 Nov 2024 10:12:53 -0600 Subject: [PATCH 06/22] add event emitter so consuming components can know when disclosure status has changed --- .../src/disclosure/disclosure.component.ts | 23 +++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/libs/components/src/disclosure/disclosure.component.ts b/libs/components/src/disclosure/disclosure.component.ts index 58c67ad0f0e..1c39a934087 100644 --- a/libs/components/src/disclosure/disclosure.component.ts +++ b/libs/components/src/disclosure/disclosure.component.ts @@ -1,4 +1,11 @@ -import { Component, HostBinding, Input, booleanAttribute } from "@angular/core"; +import { + Component, + EventEmitter, + HostBinding, + Input, + Output, + booleanAttribute, +} from "@angular/core"; let nextId = 0; @@ -8,14 +15,26 @@ let nextId = 0; template: ``, }) export class DisclosureComponent { + private _open: boolean; + + /** Emits the visibility of the disclosure content */ + @Output() onVisibilityChange = new EventEmitter(); + /** * Optionally init the disclosure in its opened state */ - @Input({ transform: booleanAttribute }) open?: boolean = false; + @Input({ transform: booleanAttribute }) set open(isOpen: boolean) { + this._open = isOpen; + this.onVisibilityChange.emit(isOpen); + } @HostBinding("class") get classList() { return this.open ? "" : "tw-hidden"; } @HostBinding("id") id = `bit-disclosure-${nextId++}`; + + get open(): boolean { + return this._open; + } } From 0b2f6207ca5188a6a8f2d266c083c4d7a01ccca7 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Fri, 8 Nov 2024 10:13:14 -0600 Subject: [PATCH 07/22] add filter badge when filters are selected and the filters are hidden --- .../vault-header-v2.component.html | 26 ++++-- .../vault-header-v2.component.spec.ts | 85 ++++++++++++++++++- .../vault-header/vault-header-v2.component.ts | 41 ++++++++- 3 files changed, 137 insertions(+), 15 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index ae626573050..da694a6bf7a 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -1,14 +1,24 @@ -
+
- +
+ + +
+ {{ numberOfFilters$ | async }} +
+
- + diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 26285460049..06df08ec90e 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -1,13 +1,16 @@ import { CommonModule } from "@angular/common"; import { ComponentFixture, TestBed } from "@angular/core/testing"; +import { FormBuilder } from "@angular/forms"; +import { By } from "@angular/platform-browser"; import { ActivatedRoute } from "@angular/router"; import { mock } from "jest-mock-extended"; import { BehaviorSubject, Subject } from "rxjs"; -import { CollectionService } from "@bitwarden/admin-console/common"; +import { Collection, CollectionService } from "@bitwarden/admin-console/common"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -16,9 +19,15 @@ import { SyncService } from "@bitwarden/common/platform/sync"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; +import { CipherType } from "@bitwarden/common/vault/enums"; +import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; import { PasswordRepromptService } from "@bitwarden/vault"; import { AutofillService } from "../../../../../autofill/services/abstractions/autofill.service"; +import { + PopupListFilter, + VaultPopupListFiltersService, +} from "../../../../../vault/popup/services/vault-popup-list-filters.service"; import { VaultHeaderV2Component } from "./vault-header-v2.component"; @@ -26,6 +35,18 @@ describe("VaultHeaderV2Component", () => { let component: VaultHeaderV2Component; let fixture: ComponentFixture; + const emptyForm: PopupListFilter = { + organization: null, + collection: null, + folder: null, + cipherType: null, + }; + + const filters$ = new BehaviorSubject(emptyForm); + + /** When it exists, returns the notification badge debug element */ + const getBadge = () => fixture.debugElement.query(By.css('[data-testid="filter-badge"]')); + beforeEach(async () => { await TestBed.configureTestingModule({ imports: [VaultHeaderV2Component, CommonModule], @@ -51,6 +72,10 @@ describe("VaultHeaderV2Component", () => { }, { provide: ActivatedRoute, useValue: { queryParams: new BehaviorSubject({}) } }, { provide: I18nService, useValue: { t: (key: string) => key } }, + { + provide: VaultPopupListFiltersService, + useValue: { filters$, filterForm: new FormBuilder().group(emptyForm) }, + }, ], }).compileComponents(); @@ -59,7 +84,61 @@ describe("VaultHeaderV2Component", () => { fixture.detectChanges(); }); - it("creates", () => { - expect(component).toBeTruthy(); + it("does not show filter badge when no filters are selected", () => { + component.disclosure.open = false; + filters$.next(emptyForm); + fixture.detectChanges(); + + expect(getBadge()).toBeNull(); + }); + + it("does not show filter badge when disclosure is open", () => { + component.disclosure.open = true; + filters$.next({ + ...emptyForm, + collection: { id: "col1" } as Collection, + }); + fixture.detectChanges(); + + expect(getBadge()).toBeNull(); + }); + + it("shows the notification badge when there are populated filters and the disclosure is closed", () => { + component.disclosure.open = false; + filters$.next({ + ...emptyForm, + collection: { id: "col1" } as Collection, + }); + fixture.detectChanges(); + + expect(getBadge()).not.toBeNull(); + }); + + it("displays the number of filters populated", () => { + component.disclosure.open = false; + filters$.next({ + ...emptyForm, + organization: { id: "org1" } as Organization, + }); + fixture.detectChanges(); + + expect(getBadge().nativeElement.textContent.trim()).toBe("1"); + + filters$.next({ + ...emptyForm, + organization: { id: "org1" } as Organization, + collection: { id: "col1" } as Collection, + }); + fixture.detectChanges(); + + expect(getBadge().nativeElement.textContent.trim()).toBe("2"); + + filters$.next({ + folder: { id: "folder1" } as FolderView, + cipherType: CipherType.Login, + organization: { id: "org1" } as Organization, + collection: { id: "col1" } as Collection, + }); + fixture.detectChanges(); }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 7f47db4c291..dce6a769d20 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,9 +1,11 @@ -import { NgClass } from "@angular/common"; -import { Component } from "@angular/core"; +import { CommonModule } from "@angular/common"; +import { Component, ViewChild } from "@angular/core"; +import { BehaviorSubject, combineLatest, map, shareReplay } from "rxjs"; import { DisclosureTriggerForDirective, IconButtonModule } from "@bitwarden/components"; import { DisclosureComponent } from "../../../../../../../../libs/components/src/disclosure/disclosure.component"; +import { VaultPopupListFiltersService } from "../../../../../vault/popup/services/vault-popup-list-filters.service"; import { VaultListFiltersComponent } from "../vault-list-filters/vault-list-filters.component"; import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.component"; @@ -17,7 +19,38 @@ import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.componen DisclosureComponent, IconButtonModule, DisclosureTriggerForDirective, - NgClass, + CommonModule, ], }) -export class VaultHeaderV2Component {} +export class VaultHeaderV2Component { + @ViewChild(DisclosureComponent) disclosure: DisclosureComponent; + + /** + * Emits the visibility status of the disclosure component. + * + * Note: defaults to `true` to match the default state in the template. + */ + private isDisclosureShown$ = new BehaviorSubject(true); + + /** + * Emits the number of applied filters. + */ + protected numberOfFilters$ = this.vaultPopupListFiltersService.filters$.pipe( + map((filters) => Object.values(filters).filter((filter) => Boolean(filter)).length), + shareReplay({ refCount: true, bufferSize: 1 }), + ); + + /** + * Emits true when the number of filters badge should be applied. + */ + protected showBadge$ = combineLatest([this.numberOfFilters$, this.isDisclosureShown$]).pipe( + map(([numberOfFilters, disclosureShown]) => numberOfFilters !== 0 && !disclosureShown), + ); + + constructor(private vaultPopupListFiltersService: VaultPopupListFiltersService) {} + + /** Updates the local status of the disclosure */ + protected disclosureVisibilityChange(isVisible: boolean) { + this.isDisclosureShown$.next(isVisible); + } +} From 1a8c58daee64ad8a9695f4d0fde0ca1819162452 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Fri, 8 Nov 2024 10:41:27 -0600 Subject: [PATCH 08/22] persist filter visibility state to disk --- .../vault-header-v2.component.html | 2 +- .../vault-header-v2.component.spec.ts | 14 +++++ .../vault-header/vault-header-v2.component.ts | 51 ++++++++++++------- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index da694a6bf7a..e5c8c3384c3 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -19,6 +19,6 @@
- + diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 06df08ec90e..561a079f1e3 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -15,6 +15,7 @@ import { AccountService } from "@bitwarden/common/auth/abstractions/account.serv import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { MessageSender } from "@bitwarden/common/platform/messaging"; +import { StateProvider } from "@bitwarden/common/platform/state"; import { SyncService } from "@bitwarden/common/platform/sync"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; @@ -43,6 +44,13 @@ describe("VaultHeaderV2Component", () => { }; const filters$ = new BehaviorSubject(emptyForm); + const state$ = new BehaviorSubject(undefined); + + // Mock state provider update + const update = (callback: () => boolean) => { + state$.next(callback()); + return Promise.resolve(); + }; /** When it exists, returns the notification badge debug element */ const getBadge = () => fixture.debugElement.query(By.css('[data-testid="filter-badge"]')); @@ -76,6 +84,10 @@ describe("VaultHeaderV2Component", () => { provide: VaultPopupListFiltersService, useValue: { filters$, filterForm: new FormBuilder().group(emptyForm) }, }, + { + provide: StateProvider, + useValue: { getGlobal: () => ({ state$, update }) }, + }, ], }).compileComponents(); @@ -140,5 +152,7 @@ describe("VaultHeaderV2Component", () => { collection: { id: "col1" } as Collection, }); fixture.detectChanges(); + + expect(getBadge().nativeElement.textContent.trim()).toBe("4"); }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index dce6a769d20..4b2a2118d8a 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,7 +1,12 @@ import { CommonModule } from "@angular/common"; -import { Component, ViewChild } from "@angular/core"; -import { BehaviorSubject, combineLatest, map, shareReplay } from "rxjs"; +import { AfterViewInit, Component, ViewChild } from "@angular/core"; +import { combineLatest, distinctUntilChanged, firstValueFrom, map, shareReplay } from "rxjs"; +import { + KeyDefinition, + StateProvider, + VAULT_SETTINGS_DISK, +} from "@bitwarden/common/platform/state"; import { DisclosureTriggerForDirective, IconButtonModule } from "@bitwarden/components"; import { DisclosureComponent } from "../../../../../../../../libs/components/src/disclosure/disclosure.component"; @@ -9,6 +14,10 @@ import { VaultPopupListFiltersService } from "../../../../../vault/popup/service import { VaultListFiltersComponent } from "../vault-list-filters/vault-list-filters.component"; import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.component"; +const FILTER_VISIBILITY_KEY = new KeyDefinition(VAULT_SETTINGS_DISK, "filterVisibility", { + deserializer: (obj) => obj, +}); + @Component({ selector: "app-vault-header-v2", templateUrl: "vault-header-v2.component.html", @@ -22,35 +31,41 @@ import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.componen CommonModule, ], }) -export class VaultHeaderV2Component { +export class VaultHeaderV2Component implements AfterViewInit { @ViewChild(DisclosureComponent) disclosure: DisclosureComponent; - /** - * Emits the visibility status of the disclosure component. - * - * Note: defaults to `true` to match the default state in the template. - */ - private isDisclosureShown$ = new BehaviorSubject(true); + /** Stored state for the visibility of the filters. */ + private filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); + + /** Emits the visibility status of the disclosure component. */ + protected isDisclosureShown$ = this.filterVisibilityState.state$.pipe( + distinctUntilChanged(), + map((visibility) => visibility ?? true), + ); - /** - * Emits the number of applied filters. - */ + /** Emits the number of applied filters. */ protected numberOfFilters$ = this.vaultPopupListFiltersService.filters$.pipe( map((filters) => Object.values(filters).filter((filter) => Boolean(filter)).length), shareReplay({ refCount: true, bufferSize: 1 }), ); - /** - * Emits true when the number of filters badge should be applied. - */ + /** Emits true when the number of filters badge should be applied. */ protected showBadge$ = combineLatest([this.numberOfFilters$, this.isDisclosureShown$]).pipe( map(([numberOfFilters, disclosureShown]) => numberOfFilters !== 0 && !disclosureShown), ); - constructor(private vaultPopupListFiltersService: VaultPopupListFiltersService) {} + constructor( + private vaultPopupListFiltersService: VaultPopupListFiltersService, + private stateProvider: StateProvider, + ) {} + + async ngAfterViewInit(): Promise { + const isDisclosureShown = await firstValueFrom(this.isDisclosureShown$); + this.disclosure.open = isDisclosureShown; + } /** Updates the local status of the disclosure */ - protected disclosureVisibilityChange(isVisible: boolean) { - this.isDisclosureShown$.next(isVisible); + protected async disclosureVisibilityChange(isVisible: boolean) { + await this.filterVisibilityState.update(() => isVisible); } } From 44e6928c79095500413202ba2cca641a2e3ed764 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Fri, 8 Nov 2024 11:23:44 -0600 Subject: [PATCH 09/22] add supporting text for the filter button --- apps/browser/src/_locales/en/messages.json | 15 +++++++++++++++ .../vault-header/vault-header-v2.component.html | 9 +++++++++ .../vault-header/vault-header-v2.component.ts | 17 +++++++++++++++++ 3 files changed, 41 insertions(+) diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index a62dac05430..91d1778e414 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -4252,6 +4252,21 @@ "filters": { "message": "Filters" }, + "filterVault": { + "message": "Filter vault" + }, + "filterApplied": { + "message": "One filter applied" + }, + "filterAppliedPlural": { + "message": "$COUNT$ filters applied", + "placeholders": { + "count": { + "content": "$1", + "example": "3" + } + } + }, "personalDetails": { "message": "Personal details" }, diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index e5c8c3384c3..4678196fc04 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -8,7 +8,16 @@ bitIconButton="bwi-sliders" [buttonType]="'muted'" [bitDisclosureTriggerFor]="disclosureRef" + [appA11yTitle]="'filterVault' | i18n" + aria-describedby="filters-applied" > +

+ {{ supportingText }} +

(VAULT_SETTINGS_DISK, "f IconButtonModule, DisclosureTriggerForDirective, CommonModule, + JslibModule, ], }) export class VaultHeaderV2Component implements AfterViewInit { @@ -54,9 +57,23 @@ export class VaultHeaderV2Component implements AfterViewInit { map(([numberOfFilters, disclosureShown]) => numberOfFilters !== 0 && !disclosureShown), ); + protected buttonSupportingText$ = this.numberOfFilters$.pipe( + map((numberOfFilters) => { + if (numberOfFilters === 0) { + return null; + } + if (numberOfFilters === 1) { + return this.i18nService.t("filterApplied"); + } + + return this.i18nService.t("filterAppliedPlural", numberOfFilters); + }), + ); + constructor( private vaultPopupListFiltersService: VaultPopupListFiltersService, private stateProvider: StateProvider, + private i18nService: I18nService, ) {} async ngAfterViewInit(): Promise { From 9270abf825cae0497a9b4f60e3752a80c8edfdcc Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Fri, 8 Nov 2024 11:27:34 -0600 Subject: [PATCH 10/22] remove extra file --- .../components/vault/vault-header-v2.component.ts | 12 ------------ 1 file changed, 12 deletions(-) delete mode 100644 apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts diff --git a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts deleted file mode 100644 index 22743a4e206..00000000000 --- a/apps/browser/src/vault/popup/components/vault/vault-header-v2.component.ts +++ /dev/null @@ -1,12 +0,0 @@ -import { Component } from "@angular/core"; - -import { VaultListFiltersComponent } from "../vault-v2/vault-list-filters/vault-list-filters.component"; -import { VaultV2SearchComponent } from "../vault-v2/vault-search/vault-v2-search.component"; - -@Component({ - selector: "app-vault-header-v2", - templateUrl: "vault-header-v2.component.html", - standalone: true, - imports: [VaultV2SearchComponent, VaultListFiltersComponent], -}) -export class VaultHeaderV2Component {} From 8d8ac571926a88fa377ff2ff6cd7ecb4304d1a38 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Fri, 8 Nov 2024 12:01:19 -0600 Subject: [PATCH 11/22] only read from stored state on component launch. - I noticed delays when trying to use stored state as the source of truth --- .../vault-header-v2.component.spec.ts | 21 +++++++++++++++---- .../vault-header/vault-header-v2.component.ts | 20 ++++++++++-------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 561a079f1e3..1055e944be9 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -47,15 +47,14 @@ describe("VaultHeaderV2Component", () => { const state$ = new BehaviorSubject(undefined); // Mock state provider update - const update = (callback: () => boolean) => { - state$.next(callback()); - return Promise.resolve(); - }; + const update = jest.fn().mockResolvedValue(undefined); /** When it exists, returns the notification badge debug element */ const getBadge = () => fixture.debugElement.query(By.css('[data-testid="filter-badge"]')); beforeEach(async () => { + update.mockClear(); + await TestBed.configureTestingModule({ imports: [VaultHeaderV2Component, CommonModule], providers: [ @@ -155,4 +154,18 @@ describe("VaultHeaderV2Component", () => { expect(getBadge().nativeElement.textContent.trim()).toBe("4"); }); + + it("reads initial state of the filter visibility from state", async () => { + state$.next(false); + + await component.ngAfterViewInit(); + + expect(component.disclosure.open).toBeFalse(); + + state$.next(true); + + await component.ngAfterViewInit(); + + expect(component.disclosure.open).toBeTrue(); + }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 2436dedfc1d..0d7de21e6f7 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,6 +1,6 @@ import { CommonModule } from "@angular/common"; import { AfterViewInit, Component, ViewChild } from "@angular/core"; -import { combineLatest, distinctUntilChanged, firstValueFrom, map, shareReplay } from "rxjs"; +import { BehaviorSubject, combineLatest, firstValueFrom, map, shareReplay } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -41,10 +41,7 @@ export class VaultHeaderV2Component implements AfterViewInit { private filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); /** Emits the visibility status of the disclosure component. */ - protected isDisclosureShown$ = this.filterVisibilityState.state$.pipe( - distinctUntilChanged(), - map((visibility) => visibility ?? true), - ); + protected isDisclosureShown$ = new BehaviorSubject(undefined); /** Emits the number of applied filters. */ protected numberOfFilters$ = this.vaultPopupListFiltersService.filters$.pipe( @@ -77,12 +74,17 @@ export class VaultHeaderV2Component implements AfterViewInit { ) {} async ngAfterViewInit(): Promise { - const isDisclosureShown = await firstValueFrom(this.isDisclosureShown$); + const isDisclosureShown = await firstValueFrom( + this.filterVisibilityState.state$.pipe(map((visibility) => visibility ?? true)), + ); this.disclosure.open = isDisclosureShown; + this.isDisclosureShown$.next(isDisclosureShown); } - /** Updates the local status of the disclosure */ - protected async disclosureVisibilityChange(isVisible: boolean) { - await this.filterVisibilityState.update(() => isVisible); + /** Updates the local and stored status of the disclosure */ + protected disclosureVisibilityChange(isVisible: boolean) { + this.isDisclosureShown$.next(isVisible); + // update stored status + void this.filterVisibilityState.update(() => isVisible); } } From 69713d03ee131f75cc8d26570d0b6448447431af Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 12 Nov 2024 08:35:29 -0600 Subject: [PATCH 12/22] use two-way data binding for change event --- libs/components/src/disclosure/disclosure.component.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libs/components/src/disclosure/disclosure.component.ts b/libs/components/src/disclosure/disclosure.component.ts index 1c39a934087..518bf5f279d 100644 --- a/libs/components/src/disclosure/disclosure.component.ts +++ b/libs/components/src/disclosure/disclosure.component.ts @@ -18,14 +18,14 @@ export class DisclosureComponent { private _open: boolean; /** Emits the visibility of the disclosure content */ - @Output() onVisibilityChange = new EventEmitter(); + @Output() openChange = new EventEmitter(); /** * Optionally init the disclosure in its opened state */ @Input({ transform: booleanAttribute }) set open(isOpen: boolean) { this._open = isOpen; - this.onVisibilityChange.emit(isOpen); + this.openChange.emit(isOpen); } @HostBinding("class") get classList() { From 07e9ee49cc5eb81966f79c7c0414f47ee242276c Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 12 Nov 2024 08:40:58 -0600 Subject: [PATCH 13/22] update vault headers to use two way data binds from disclosure component - also adjust consuming changes --- .../vault-header-v2.component.html | 2 +- .../vault-header-v2.component.spec.ts | 20 ++++---- .../vault-header/vault-header-v2.component.ts | 46 +++++++++++++------ 3 files changed, 42 insertions(+), 26 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 4678196fc04..1c4ddba8d52 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -28,6 +28,6 @@
- + diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 1055e944be9..e8607edce10 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -44,7 +44,7 @@ describe("VaultHeaderV2Component", () => { }; const filters$ = new BehaviorSubject(emptyForm); - const state$ = new BehaviorSubject(undefined); + const state$ = new BehaviorSubject(null); // Mock state provider update const update = jest.fn().mockResolvedValue(undefined); @@ -96,7 +96,7 @@ describe("VaultHeaderV2Component", () => { }); it("does not show filter badge when no filters are selected", () => { - component.disclosure.open = false; + component["disclosureVisibility"](false); filters$.next(emptyForm); fixture.detectChanges(); @@ -104,7 +104,7 @@ describe("VaultHeaderV2Component", () => { }); it("does not show filter badge when disclosure is open", () => { - component.disclosure.open = true; + component["disclosureVisibility"](true); filters$.next({ ...emptyForm, collection: { id: "col1" } as Collection, @@ -114,8 +114,8 @@ describe("VaultHeaderV2Component", () => { expect(getBadge()).toBeNull(); }); - it("shows the notification badge when there are populated filters and the disclosure is closed", () => { - component.disclosure.open = false; + it("shows the notification badge when there are populated filters and the disclosure is closed", async () => { + component["disclosureVisibility"](false); filters$.next({ ...emptyForm, collection: { id: "col1" } as Collection, @@ -126,11 +126,11 @@ describe("VaultHeaderV2Component", () => { }); it("displays the number of filters populated", () => { - component.disclosure.open = false; filters$.next({ ...emptyForm, organization: { id: "org1" } as Organization, }); + component["disclosureVisibility"](false); fixture.detectChanges(); expect(getBadge().nativeElement.textContent.trim()).toBe("1"); @@ -158,14 +158,14 @@ describe("VaultHeaderV2Component", () => { it("reads initial state of the filter visibility from state", async () => { state$.next(false); - await component.ngAfterViewInit(); + await component.ngOnInit(); - expect(component.disclosure.open).toBeFalse(); + expect(component["isDisclosureShown$"].value).toBeFalse(); state$.next(true); - await component.ngAfterViewInit(); + await component.ngOnInit(); - expect(component.disclosure.open).toBeTrue(); + expect(component["isDisclosureShown$"].value).toBeTrue(); }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 0d7de21e6f7..25ee56fb8e9 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,6 +1,7 @@ import { CommonModule } from "@angular/common"; -import { AfterViewInit, Component, ViewChild } from "@angular/core"; -import { BehaviorSubject, combineLatest, firstValueFrom, map, shareReplay } from "rxjs"; +import { ChangeDetectorRef, Component, DestroyRef, inject, OnInit, ViewChild } from "@angular/core"; +import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; +import { BehaviorSubject, combineLatest, first, map, shareReplay } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -34,14 +35,14 @@ const FILTER_VISIBILITY_KEY = new KeyDefinition(VAULT_SETTINGS_DISK, "f JslibModule, ], }) -export class VaultHeaderV2Component implements AfterViewInit { +export class VaultHeaderV2Component implements OnInit { @ViewChild(DisclosureComponent) disclosure: DisclosureComponent; /** Stored state for the visibility of the filters. */ private filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); /** Emits the visibility status of the disclosure component. */ - protected isDisclosureShown$ = new BehaviorSubject(undefined); + protected isDisclosureShown$ = new BehaviorSubject(null); /** Emits the number of applied filters. */ protected numberOfFilters$ = this.vaultPopupListFiltersService.filters$.pipe( @@ -67,24 +68,39 @@ export class VaultHeaderV2Component implements AfterViewInit { }), ); + private destroyRef = inject(DestroyRef); + constructor( private vaultPopupListFiltersService: VaultPopupListFiltersService, private stateProvider: StateProvider, private i18nService: I18nService, + private changeDetectorRef: ChangeDetectorRef, ) {} - async ngAfterViewInit(): Promise { - const isDisclosureShown = await firstValueFrom( - this.filterVisibilityState.state$.pipe(map((visibility) => visibility ?? true)), - ); - this.disclosure.open = isDisclosureShown; - this.isDisclosureShown$.next(isDisclosureShown); + ngOnInit(): void { + // Get the initial visibility from stored state + this.filterVisibilityState.state$ + .pipe( + first(), + takeUntilDestroyed(this.destroyRef), + map((visibility) => visibility ?? true), + ) + .subscribe((showFilters) => { + this.disclosure.open = showFilters; + this.disclosureVisibility(showFilters); + // Force change detection after updating from state, + // avoids `ExpressionChangedAfterItHasBeenCheckedError`. + this.changeDetectorRef.detectChanges(); + }); } - /** Updates the local and stored status of the disclosure */ - protected disclosureVisibilityChange(isVisible: boolean) { - this.isDisclosureShown$.next(isVisible); - // update stored status - void this.filterVisibilityState.update(() => isVisible); + protected disclosureVisibility(isShown: boolean) { + // If local state is already up to date with the disclosure, exit early. + if (this.isDisclosureShown$.value === isShown) { + return; + } + + this.isDisclosureShown$.next(isShown); + void this.filterVisibilityState.update(() => isShown); } } From 9bd3ff6e164c674312ab751e2c4a2efab0ccb67f Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 12 Nov 2024 08:42:50 -0600 Subject: [PATCH 14/22] add border thickness --- .../vault-v2/vault-header/vault-header-v2.component.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 1c4ddba8d52..7e78c504bdf 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -21,7 +21,7 @@
{{ numberOfFilters$ | async }} From fc50caffbaa9efe0a2aecf52463b16fd22f738d3 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 12 Nov 2024 08:47:21 -0600 Subject: [PATCH 15/22] add ticket to the FIXME --- .../vault-v2/vault-header/vault-header-v2.component.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 7e78c504bdf..8d2d65318d2 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -18,7 +18,7 @@ > {{ supportingText }}

- +
Date: Tue, 12 Nov 2024 09:25:51 -0600 Subject: [PATCH 16/22] move number of filters observable into service --- .../vault-header-v2.component.html | 2 +- .../vault-header-v2.component.spec.ts | 45 +++++++------------ .../vault-header/vault-header-v2.component.ts | 17 +++---- .../vault-popup-list-filters.service.spec.ts | 14 ++++++ .../vault-popup-list-filters.service.ts | 7 +++ 5 files changed, 44 insertions(+), 41 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 8d2d65318d2..469069f797a 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -24,7 +24,7 @@ class="tw-flex tw-items-center tw-justify-center tw-z-10 tw-absolute tw-rounded-full tw-h-[15px] tw-w-[15px] tw-top-[1px] tw-right-[1px] tw-text-[#c01176] tw-text-[8px] tw-border-[#c01176] tw-border-[0.5px] tw-border-solid tw-bg-[#FFEAFA] tw-leading-normal" data-testid="filter-badge" > - {{ numberOfFilters$ | async }} + {{ numberOfAppliedFilters$ | async }}
diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index e8607edce10..e74268c5a00 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -6,11 +6,10 @@ import { ActivatedRoute } from "@angular/router"; import { mock } from "jest-mock-extended"; import { BehaviorSubject, Subject } from "rxjs"; -import { Collection, CollectionService } from "@bitwarden/admin-console/common"; +import { CollectionService } from "@bitwarden/admin-console/common"; import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; -import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; @@ -20,8 +19,6 @@ import { SyncService } from "@bitwarden/common/platform/sync"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault-settings/vault-settings.service"; -import { CipherType } from "@bitwarden/common/vault/enums"; -import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; import { PasswordRepromptService } from "@bitwarden/vault"; import { AutofillService } from "../../../../../autofill/services/abstractions/autofill.service"; @@ -43,7 +40,7 @@ describe("VaultHeaderV2Component", () => { cipherType: null, }; - const filters$ = new BehaviorSubject(emptyForm); + const numberOfAppliedFilters$ = new BehaviorSubject(0); const state$ = new BehaviorSubject(null); // Mock state provider update @@ -81,7 +78,11 @@ describe("VaultHeaderV2Component", () => { { provide: I18nService, useValue: { t: (key: string) => key } }, { provide: VaultPopupListFiltersService, - useValue: { filters$, filterForm: new FormBuilder().group(emptyForm) }, + useValue: { + numberOfAppliedFilters$, + filters$: new BehaviorSubject(emptyForm), + filterForm: new FormBuilder().group(emptyForm), + }, }, { provide: StateProvider, @@ -97,7 +98,7 @@ describe("VaultHeaderV2Component", () => { it("does not show filter badge when no filters are selected", () => { component["disclosureVisibility"](false); - filters$.next(emptyForm); + numberOfAppliedFilters$.next(0); fixture.detectChanges(); expect(getBadge()).toBeNull(); @@ -105,10 +106,7 @@ describe("VaultHeaderV2Component", () => { it("does not show filter badge when disclosure is open", () => { component["disclosureVisibility"](true); - filters$.next({ - ...emptyForm, - collection: { id: "col1" } as Collection, - }); + numberOfAppliedFilters$.next(1); fixture.detectChanges(); expect(getBadge()).toBeNull(); @@ -116,40 +114,27 @@ describe("VaultHeaderV2Component", () => { it("shows the notification badge when there are populated filters and the disclosure is closed", async () => { component["disclosureVisibility"](false); - filters$.next({ - ...emptyForm, - collection: { id: "col1" } as Collection, - }); + numberOfAppliedFilters$.next(1); fixture.detectChanges(); expect(getBadge()).not.toBeNull(); }); it("displays the number of filters populated", () => { - filters$.next({ - ...emptyForm, - organization: { id: "org1" } as Organization, - }); + numberOfAppliedFilters$.next(1); component["disclosureVisibility"](false); fixture.detectChanges(); expect(getBadge().nativeElement.textContent.trim()).toBe("1"); - filters$.next({ - ...emptyForm, - organization: { id: "org1" } as Organization, - collection: { id: "col1" } as Collection, - }); + numberOfAppliedFilters$.next(2); + fixture.detectChanges(); expect(getBadge().nativeElement.textContent.trim()).toBe("2"); - filters$.next({ - folder: { id: "folder1" } as FolderView, - cipherType: CipherType.Login, - organization: { id: "org1" } as Organization, - collection: { id: "col1" } as Collection, - }); + numberOfAppliedFilters$.next(4); + fixture.detectChanges(); expect(getBadge().nativeElement.textContent.trim()).toBe("4"); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 25ee56fb8e9..992e4153653 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,7 +1,7 @@ import { CommonModule } from "@angular/common"; import { ChangeDetectorRef, Component, DestroyRef, inject, OnInit, ViewChild } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { BehaviorSubject, combineLatest, first, map, shareReplay } from "rxjs"; +import { BehaviorSubject, combineLatest, first, map } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; @@ -44,18 +44,15 @@ export class VaultHeaderV2Component implements OnInit { /** Emits the visibility status of the disclosure component. */ protected isDisclosureShown$ = new BehaviorSubject(null); - /** Emits the number of applied filters. */ - protected numberOfFilters$ = this.vaultPopupListFiltersService.filters$.pipe( - map((filters) => Object.values(filters).filter((filter) => Boolean(filter)).length), - shareReplay({ refCount: true, bufferSize: 1 }), - ); + protected numberOfAppliedFilters$ = this.vaultPopupListFiltersService.numberOfAppliedFilters$; /** Emits true when the number of filters badge should be applied. */ - protected showBadge$ = combineLatest([this.numberOfFilters$, this.isDisclosureShown$]).pipe( - map(([numberOfFilters, disclosureShown]) => numberOfFilters !== 0 && !disclosureShown), - ); + protected showBadge$ = combineLatest([ + this.numberOfAppliedFilters$, + this.isDisclosureShown$, + ]).pipe(map(([numberOfFilters, disclosureShown]) => numberOfFilters !== 0 && !disclosureShown)); - protected buttonSupportingText$ = this.numberOfFilters$.pipe( + protected buttonSupportingText$ = this.numberOfAppliedFilters$.pipe( map((numberOfFilters) => { if (numberOfFilters === 0) { return null; diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts index 02ad7375f6a..dfb8f02881f 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts @@ -102,6 +102,20 @@ describe("VaultPopupListFiltersService", () => { }); }); + describe("numberOfAppliedFilters$", () => { + it("updates as the form value changes", (done) => { + service.numberOfAppliedFilters$.subscribe((number) => { + expect(number).toBe(2); + done(); + }); + + service.filterForm.patchValue({ + organization: { id: "1234" } as Organization, + folder: { id: "folder11" } as FolderView, + }); + }); + }); + describe("organizations$", () => { it('does not add "myVault" to the list of organizations when there are no organizations', (done) => { memberOrganizations$.next([]); diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts index 590807cff60..0aa48f064a6 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts @@ -6,6 +6,7 @@ import { distinctUntilChanged, map, Observable, + shareReplay, startWith, switchMap, tap, @@ -66,6 +67,12 @@ export class VaultPopupListFiltersService { startWith(INITIAL_FILTERS), ) as Observable; + /** Emits the number of applied filters. */ + numberOfAppliedFilters$ = this.filters$.pipe( + map((filters) => Object.values(filters).filter((filter) => Boolean(filter)).length), + shareReplay({ refCount: true, bufferSize: 1 }), + ); + /** * Static list of ciphers views used in synchronous context */ From 5b98732214621c20cfdfb35c82ba038ded9f29b6 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 12 Nov 2024 10:00:32 -0600 Subject: [PATCH 17/22] move state coordination into filter service --- .../vault-header-v2.component.spec.ts | 1 + .../vault-header/vault-header-v2.component.ts | 17 ++--------------- .../vault-popup-list-filters.service.spec.ts | 5 +++++ .../vault-popup-list-filters.service.ts | 13 +++++++++++++ 4 files changed, 21 insertions(+), 15 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index e74268c5a00..76455861b25 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -82,6 +82,7 @@ describe("VaultHeaderV2Component", () => { numberOfAppliedFilters$, filters$: new BehaviorSubject(emptyForm), filterForm: new FormBuilder().group(emptyForm), + filterVisibilityState: { state$, update }, }, }, { diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 992e4153653..e5534a06c36 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -5,11 +5,6 @@ import { BehaviorSubject, combineLatest, first, map } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { - KeyDefinition, - StateProvider, - VAULT_SETTINGS_DISK, -} from "@bitwarden/common/platform/state"; import { DisclosureTriggerForDirective, IconButtonModule } from "@bitwarden/components"; import { DisclosureComponent } from "../../../../../../../../libs/components/src/disclosure/disclosure.component"; @@ -17,10 +12,6 @@ import { VaultPopupListFiltersService } from "../../../../../vault/popup/service import { VaultListFiltersComponent } from "../vault-list-filters/vault-list-filters.component"; import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.component"; -const FILTER_VISIBILITY_KEY = new KeyDefinition(VAULT_SETTINGS_DISK, "filterVisibility", { - deserializer: (obj) => obj, -}); - @Component({ selector: "app-vault-header-v2", templateUrl: "vault-header-v2.component.html", @@ -38,9 +29,6 @@ const FILTER_VISIBILITY_KEY = new KeyDefinition(VAULT_SETTINGS_DISK, "f export class VaultHeaderV2Component implements OnInit { @ViewChild(DisclosureComponent) disclosure: DisclosureComponent; - /** Stored state for the visibility of the filters. */ - private filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); - /** Emits the visibility status of the disclosure component. */ protected isDisclosureShown$ = new BehaviorSubject(null); @@ -69,14 +57,13 @@ export class VaultHeaderV2Component implements OnInit { constructor( private vaultPopupListFiltersService: VaultPopupListFiltersService, - private stateProvider: StateProvider, private i18nService: I18nService, private changeDetectorRef: ChangeDetectorRef, ) {} ngOnInit(): void { // Get the initial visibility from stored state - this.filterVisibilityState.state$ + this.vaultPopupListFiltersService.filterVisibilityState.state$ .pipe( first(), takeUntilDestroyed(this.destroyRef), @@ -98,6 +85,6 @@ export class VaultHeaderV2Component implements OnInit { } this.isDisclosureShown$.next(isShown); - void this.filterVisibilityState.update(() => isShown); + void this.vaultPopupListFiltersService.filterVisibilityState.update(() => isShown); } } diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts index dfb8f02881f..32ad96db2ef 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts @@ -9,6 +9,7 @@ import { PolicyType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { ProductTierType } from "@bitwarden/common/billing/enums"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { StateProvider } from "@bitwarden/common/platform/state"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherType } from "@bitwarden/common/vault/enums"; @@ -83,6 +84,10 @@ describe("VaultPopupListFiltersService", () => { provide: PolicyService, useValue: policyService, }, + { + provide: StateProvider, + useValue: { getGlobal: () => ({}) }, + }, { provide: FormBuilder, useClass: FormBuilder }, ], }); diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts index 0aa48f064a6..002b79a0fc2 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts @@ -21,6 +21,11 @@ import { Organization } from "@bitwarden/common/admin-console/models/domain/orga import { ProductTierType } from "@bitwarden/common/billing/enums"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; +import { + KeyDefinition, + StateProvider, + VAULT_SETTINGS_DISK, +} from "@bitwarden/common/platform/state"; import { CipherService } from "@bitwarden/common/vault/abstractions/cipher.service"; import { FolderService } from "@bitwarden/common/vault/abstractions/folder/folder.service.abstraction"; import { CipherType } from "@bitwarden/common/vault/enums"; @@ -30,6 +35,10 @@ import { FolderView } from "@bitwarden/common/vault/models/view/folder.view"; import { ServiceUtils } from "@bitwarden/common/vault/service-utils"; import { ChipSelectOption } from "@bitwarden/components"; +const FILTER_VISIBILITY_KEY = new KeyDefinition(VAULT_SETTINGS_DISK, "filterVisibility", { + deserializer: (obj) => obj, +}); + /** All available cipher filters */ export type PopupListFilter = { organization: Organization | null; @@ -73,6 +82,9 @@ export class VaultPopupListFiltersService { shareReplay({ refCount: true, bufferSize: 1 }), ); + /** Stored state for the visibility of the filters. */ + filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); + /** * Static list of ciphers views used in synchronous context */ @@ -96,6 +108,7 @@ export class VaultPopupListFiltersService { private collectionService: CollectionService, private formBuilder: FormBuilder, private policyService: PolicyService, + private stateProvider: StateProvider, ) { this.filterForm.controls.organization.valueChanges .pipe(takeUntilDestroyed()) From 3a9eb5f88e1dcebc7511fca65c9e2c9c8dac0919 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 13 Nov 2024 11:28:19 -0600 Subject: [PATCH 18/22] only expose state and update methods from filter service --- .../vault-header-v2.component.spec.ts | 3 ++- .../vault-header/vault-header-v2.component.ts | 4 +-- .../vault-popup-list-filters.service.spec.ts | 25 ++++++++++++++++++- .../vault-popup-list-filters.service.ts | 10 +++++++- 4 files changed, 37 insertions(+), 5 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 76455861b25..0df5f8dba52 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -82,7 +82,8 @@ describe("VaultHeaderV2Component", () => { numberOfAppliedFilters$, filters$: new BehaviorSubject(emptyForm), filterForm: new FormBuilder().group(emptyForm), - filterVisibilityState: { state$, update }, + filterVisibilityState$: state$, + updateFilterVisibility: update, }, }, { diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index e5534a06c36..62c164762a9 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -63,7 +63,7 @@ export class VaultHeaderV2Component implements OnInit { ngOnInit(): void { // Get the initial visibility from stored state - this.vaultPopupListFiltersService.filterVisibilityState.state$ + this.vaultPopupListFiltersService.filterVisibilityState$ .pipe( first(), takeUntilDestroyed(this.destroyRef), @@ -85,6 +85,6 @@ export class VaultHeaderV2Component implements OnInit { } this.isDisclosureShown$.next(isShown); - void this.vaultPopupListFiltersService.filterVisibilityState.update(() => isShown); + void this.vaultPopupListFiltersService.updateFilterVisibility(isShown); } } diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts index 32ad96db2ef..580514de610 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.spec.ts @@ -51,6 +51,9 @@ describe("VaultPopupListFiltersService", () => { policyAppliesToActiveUser$: jest.fn(() => policyAppliesToActiveUser$), }; + const state$ = new BehaviorSubject(false); + const update = jest.fn().mockResolvedValue(undefined); + beforeEach(() => { memberOrganizations$.next([]); decryptedCollections$.next([]); @@ -86,7 +89,7 @@ describe("VaultPopupListFiltersService", () => { }, { provide: StateProvider, - useValue: { getGlobal: () => ({}) }, + useValue: { getGlobal: () => ({ state$, update }) }, }, { provide: FormBuilder, useClass: FormBuilder }, ], @@ -470,4 +473,24 @@ describe("VaultPopupListFiltersService", () => { }); }); }); + + describe("filterVisibilityState", () => { + it("exposes stored state through filterVisibilityState$", (done) => { + state$.next(true); + + service.filterVisibilityState$.subscribe((filterVisibility) => { + expect(filterVisibility).toBeTrue(); + done(); + }); + }); + + it("updates stored filter state", async () => { + await service.updateFilterVisibility(false); + + expect(update).toHaveBeenCalledOnce(); + // Get callback passed to `update` + const updateCallback = update.mock.calls[0][0]; + expect(updateCallback()).toBe(false); + }); + }); }); diff --git a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts index 002b79a0fc2..32eaeb27d4e 100644 --- a/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts +++ b/apps/browser/src/vault/popup/services/vault-popup-list-filters.service.ts @@ -83,7 +83,7 @@ export class VaultPopupListFiltersService { ); /** Stored state for the visibility of the filters. */ - filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); + private filterVisibilityState = this.stateProvider.getGlobal(FILTER_VISIBILITY_KEY); /** * Static list of ciphers views used in synchronous context @@ -115,6 +115,9 @@ export class VaultPopupListFiltersService { .subscribe(this.validateOrganizationChange.bind(this)); } + /** Stored state for the visibility of the filters. */ + filterVisibilityState$ = this.filterVisibilityState.state$; + /** * Observable whose value is a function that filters an array of `CipherView` objects based on the current filters */ @@ -352,6 +355,11 @@ export class VaultPopupListFiltersService { ), ); + /** Updates the stored state for filter visibility. */ + async updateFilterVisibility(isVisible: boolean): Promise { + await this.filterVisibilityState.update(() => isVisible); + } + /** * Converts the given item into the `ChipSelectOption` structure */ From 7923b1d010e3983fc9d6b2bd9ddf679339f99f2c Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Wed, 13 Nov 2024 14:44:26 -0600 Subject: [PATCH 19/22] simplify observables to avoid needed state lifecycle methods --- .../vault-header-v2.component.html | 12 +++-- .../vault-header-v2.component.spec.ts | 29 +++++------- .../vault-header/vault-header-v2.component.ts | 47 ++++++------------- 3 files changed, 36 insertions(+), 52 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 469069f797a..3cf19f2750d 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -1,4 +1,4 @@ -
+
@@ -28,6 +28,12 @@
- - + +
+ +
diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 0df5f8dba52..7805c5db561 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -41,7 +41,7 @@ describe("VaultHeaderV2Component", () => { }; const numberOfAppliedFilters$ = new BehaviorSubject(0); - const state$ = new BehaviorSubject(null); + const state$ = new Subject(); // Mock state provider update const update = jest.fn().mockResolvedValue(undefined); @@ -99,7 +99,7 @@ describe("VaultHeaderV2Component", () => { }); it("does not show filter badge when no filters are selected", () => { - component["disclosureVisibility"](false); + state$.next(false); numberOfAppliedFilters$.next(0); fixture.detectChanges(); @@ -107,7 +107,7 @@ describe("VaultHeaderV2Component", () => { }); it("does not show filter badge when disclosure is open", () => { - component["disclosureVisibility"](true); + state$.next(true); numberOfAppliedFilters$.next(1); fixture.detectChanges(); @@ -115,7 +115,7 @@ describe("VaultHeaderV2Component", () => { }); it("shows the notification badge when there are populated filters and the disclosure is closed", async () => { - component["disclosureVisibility"](false); + state$.next(false); numberOfAppliedFilters$.next(1); fixture.detectChanges(); @@ -124,7 +124,7 @@ describe("VaultHeaderV2Component", () => { it("displays the number of filters populated", () => { numberOfAppliedFilters$.next(1); - component["disclosureVisibility"](false); + state$.next(false); fixture.detectChanges(); expect(getBadge().nativeElement.textContent.trim()).toBe("1"); @@ -142,17 +142,14 @@ describe("VaultHeaderV2Component", () => { expect(getBadge().nativeElement.textContent.trim()).toBe("4"); }); - it("reads initial state of the filter visibility from state", async () => { - state$.next(false); - - await component.ngOnInit(); - - expect(component["isDisclosureShown$"].value).toBeFalse(); - - state$.next(true); - - await component.ngOnInit(); + it("defaults the initial state to true", (done) => { + // The initial value of the `state$` variable above is undefined + component["initialDisclosureVisibility$"].subscribe((initialVisibility) => { + expect(initialVisibility).toBeTrue(); + done(); + }); - expect(component["isDisclosureShown$"].value).toBeTrue(); + // Update the state to null + state$.next(null); }); }); diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index 62c164762a9..c1a81980086 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -1,13 +1,13 @@ import { CommonModule } from "@angular/common"; -import { ChangeDetectorRef, Component, DestroyRef, inject, OnInit, ViewChild } from "@angular/core"; -import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; -import { BehaviorSubject, combineLatest, first, map } from "rxjs"; +import { Component, inject, NgZone, ViewChild } from "@angular/core"; +import { combineLatest, map, take } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { DisclosureTriggerForDirective, IconButtonModule } from "@bitwarden/components"; import { DisclosureComponent } from "../../../../../../../../libs/components/src/disclosure/disclosure.component"; +import { runInsideAngular } from "../../../../../platform/browser/run-inside-angular.operator"; import { VaultPopupListFiltersService } from "../../../../../vault/popup/services/vault-popup-list-filters.service"; import { VaultListFiltersComponent } from "../vault-list-filters/vault-list-filters.component"; import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.component"; @@ -26,11 +26,18 @@ import { VaultV2SearchComponent } from "../vault-search/vault-v2-search.componen JslibModule, ], }) -export class VaultHeaderV2Component implements OnInit { +export class VaultHeaderV2Component { @ViewChild(DisclosureComponent) disclosure: DisclosureComponent; /** Emits the visibility status of the disclosure component. */ - protected isDisclosureShown$ = new BehaviorSubject(null); + // protected isDisclosureShown$ = new BehaviorSubject(null); + protected isDisclosureShown$ = this.vaultPopupListFiltersService.filterVisibilityState$.pipe( + runInsideAngular(inject(NgZone)), // Browser state updates can happen outside of `ngZone` + map((v) => v ?? true), + ); + + // Only use the first value to avoid an infinite loop from two-way binding + protected initialDisclosureVisibility$ = this.isDisclosureShown$.pipe(take(1)); protected numberOfAppliedFilters$ = this.vaultPopupListFiltersService.numberOfAppliedFilters$; @@ -53,38 +60,12 @@ export class VaultHeaderV2Component implements OnInit { }), ); - private destroyRef = inject(DestroyRef); - constructor( private vaultPopupListFiltersService: VaultPopupListFiltersService, private i18nService: I18nService, - private changeDetectorRef: ChangeDetectorRef, ) {} - ngOnInit(): void { - // Get the initial visibility from stored state - this.vaultPopupListFiltersService.filterVisibilityState$ - .pipe( - first(), - takeUntilDestroyed(this.destroyRef), - map((visibility) => visibility ?? true), - ) - .subscribe((showFilters) => { - this.disclosure.open = showFilters; - this.disclosureVisibility(showFilters); - // Force change detection after updating from state, - // avoids `ExpressionChangedAfterItHasBeenCheckedError`. - this.changeDetectorRef.detectChanges(); - }); - } - - protected disclosureVisibility(isShown: boolean) { - // If local state is already up to date with the disclosure, exit early. - if (this.isDisclosureShown$.value === isShown) { - return; - } - - this.isDisclosureShown$.next(isShown); - void this.vaultPopupListFiltersService.updateFilterVisibility(isShown); + async toggleFilters(isShown: boolean) { + await this.vaultPopupListFiltersService.updateFilterVisibility(isShown); } } From 6e00bc561bc0f4552efb90dbb6bf97d3db0dfa39 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 14 Nov 2024 08:35:17 -0600 Subject: [PATCH 20/22] remove comment --- .../vault-v2/vault-header/vault-header-v2.component.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts index c1a81980086..c7183f6fa28 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.ts @@ -30,7 +30,6 @@ export class VaultHeaderV2Component { @ViewChild(DisclosureComponent) disclosure: DisclosureComponent; /** Emits the visibility status of the disclosure component. */ - // protected isDisclosureShown$ = new BehaviorSubject(null); protected isDisclosureShown$ = this.vaultPopupListFiltersService.filterVisibilityState$.pipe( runInsideAngular(inject(NgZone)), // Browser state updates can happen outside of `ngZone` map((v) => v ?? true), From 5a7183e3bce5ebf1a78c180f2ca2c25393eb8644 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 14 Nov 2024 08:49:39 -0600 Subject: [PATCH 21/22] fix test imports --- .../vault-header/vault-header-v2.component.spec.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts index 7805c5db561..38ec6056d19 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.spec.ts @@ -12,6 +12,7 @@ import { OrganizationService } from "@bitwarden/common/admin-console/abstraction import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { LogService } from "@bitwarden/common/platform/abstractions/log.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { MessageSender } from "@bitwarden/common/platform/messaging"; import { StateProvider } from "@bitwarden/common/platform/state"; @@ -22,6 +23,7 @@ import { VaultSettingsService } from "@bitwarden/common/vault/abstractions/vault import { PasswordRepromptService } from "@bitwarden/vault"; import { AutofillService } from "../../../../../autofill/services/abstractions/autofill.service"; +import { VaultPopupItemsService } from "../../../../../vault/popup/services/vault-popup-items.service"; import { PopupListFilter, VaultPopupListFiltersService, @@ -70,6 +72,11 @@ describe("VaultHeaderV2Component", () => { { provide: PasswordRepromptService, useValue: mock() }, { provide: MessageSender, useValue: mock() }, { provide: AccountService, useValue: mock() }, + { provide: LogService, useValue: mock() }, + { + provide: VaultPopupItemsService, + useValue: mock({ latestSearchText$: new BehaviorSubject("") }), + }, { provide: SyncService, useValue: mock({ activeUserLastSync$: () => new Subject() }), From 64782dbf511e946b9e885219c4324b20e1a5e5e7 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 18 Nov 2024 15:53:43 -0600 Subject: [PATCH 22/22] update badge colors --- .../vault-v2/vault-header/vault-header-v2.component.html | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html index 3cf19f2750d..05deeec0d3d 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/vault-header/vault-header-v2.component.html @@ -18,10 +18,9 @@ > {{ supportingText }}

-
{{ numberOfAppliedFilters$ | async }}