1
0
mirror of https://github.com/bitwarden/browser synced 2025-12-06 00:13:28 +00:00

[CL-736] migrate chip select to use signals (#17136)

* migrate chip select to use signals

* Have Claude address feedback and create spec file

* remove eslint disable comment

* fix failing tests

* remove unnecessary tests

* improved documentation

* remove unnecessary test logic

* consolidate tests and remove fragile selectors
This commit is contained in:
Bryan Cunningham
2025-11-13 15:53:05 -05:00
committed by GitHub
parent 9586057a32
commit ccf7bb1753
3 changed files with 545 additions and 56 deletions

View File

@@ -3,11 +3,11 @@
class="tw-inline-flex tw-items-center tw-rounded-full tw-w-full tw-border-solid tw-border tw-gap-1.5 tw-group/chip-select"
[ngClass]="{
'tw-bg-text-muted hover:tw-bg-secondary-700 tw-text-contrast hover:!tw-border-secondary-700':
selectedOption && !disabled,
selectedOption && !disabled(),
'tw-bg-transparent hover:tw-border-secondary-700 !tw-text-muted hover:tw-bg-secondary-100':
!selectedOption && !disabled,
'tw-bg-secondary-300 tw-text-muted tw-border-transparent': disabled,
'tw-border-text-muted': !disabled,
!selectedOption && !disabled(),
'tw-bg-secondary-300 tw-text-muted tw-border-transparent': disabled(),
'tw-border-text-muted': !disabled(),
'tw-ring-2 tw-ring-primary-600 tw-ring-offset-1': focusVisibleWithin(),
}"
>
@@ -17,11 +17,11 @@
class="tw-inline-flex tw-gap-1.5 tw-items-center tw-justify-between tw-bg-transparent hover:tw-bg-transparent tw-border-none tw-outline-none tw-w-full tw-py-1 tw-ps-3 last:tw-pe-3 [&:not(:last-child)]:tw-pe-0 tw-truncate tw-text-[color:inherit] tw-text-[length:inherit]"
data-fvw-target
[ngClass]="{
'tw-cursor-not-allowed': disabled,
'group-hover/chip-select:tw-text-secondary-700': !selectedOption && !disabled,
'tw-cursor-not-allowed': disabled(),
'group-hover/chip-select:tw-text-secondary-700': !selectedOption && !disabled(),
}"
[bitMenuTriggerFor]="menu"
[disabled]="disabled"
[disabled]="disabled()"
[title]="label"
#menuTrigger="menuTrigger"
(click)="setMenuWidth()"
@@ -45,10 +45,10 @@
<button
type="button"
[attr.aria-label]="'removeItem' | i18n: label"
[disabled]="disabled"
[disabled]="disabled()"
class="tw-bg-transparent hover:tw-bg-hover-contrast tw-outline-none tw-rounded-full tw-py-0.5 tw-px-1 tw-me-1 tw-text-[color:inherit] tw-text-[length:inherit] tw-border-solid tw-border tw-border-transparent tw-flex tw-items-center tw-justify-center focus-visible:tw-ring-2 tw-ring-text-contrast hover:disabled:tw-bg-transparent"
[ngClass]="{
'tw-cursor-not-allowed': disabled,
'tw-cursor-not-allowed': disabled(),
}"
(click)="clear()"
>

View File

@@ -0,0 +1,486 @@
import { ChangeDetectionStrategy, Component, signal } from "@angular/core";
import { ComponentFixture, TestBed } from "@angular/core/testing";
import { FormControl } from "@angular/forms";
import { By } from "@angular/platform-browser";
import { NoopAnimationsModule } from "@angular/platform-browser/animations";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { MenuTriggerForDirective } from "../menu";
import { ChipSelectComponent, ChipSelectOption } from "./chip-select.component";
const mockI18nService = {
t: (key: string, ...args: string[]) => {
if (key === "removeItem") {
return `Remove ${args[0]}`;
}
if (key === "backTo") {
return `Back to ${args[0]}`;
}
if (key === "viewItemsIn") {
return `View items in ${args[0]}`;
}
return key;
},
};
describe("ChipSelectComponent", () => {
let component: ChipSelectComponent<string>;
let fixture: ComponentFixture<TestApp>;
const testOptions: ChipSelectOption<string>[] = [
{ label: "Option 1", value: "opt1", icon: "bwi-folder" },
{ label: "Option 2", value: "opt2" },
{
label: "Parent Option",
value: "parent",
children: [
{ label: "Child 1", value: "child1" },
{ label: "Child 2", value: "child2" },
],
},
];
const getMenuTriggerDirective = () => {
const buttonDebugElement = fixture.debugElement.query(By.directive(MenuTriggerForDirective));
return buttonDebugElement?.injector.get(MenuTriggerForDirective);
};
const getBitMenuPanel = () => document.querySelector(".bit-menu-panel");
const getChipButton = () =>
fixture.debugElement.query(By.css("[data-fvw-target]"))?.nativeElement as HTMLButtonElement;
const getClearButton = () =>
fixture.debugElement.query(By.css('button[aria-label^="Remove"]'))
?.nativeElement as HTMLButtonElement;
beforeEach(async () => {
await TestBed.configureTestingModule({
imports: [TestApp, NoopAnimationsModule],
providers: [{ provide: I18nService, useValue: mockI18nService }],
}).compileComponents();
fixture = TestBed.createComponent(TestApp);
fixture.detectChanges();
component = fixture.debugElement.query(By.directive(ChipSelectComponent)).componentInstance;
fixture.detectChanges();
});
describe("User-Facing Behavior", () => {
it("should display placeholder text when no option is selected", () => {
expect(getChipButton().textContent).toContain("Select an option");
});
it("should display placeholder icon when no option is selected", () => {
const icon = fixture.debugElement.query(By.css('i[aria-hidden="true"]'));
expect(icon).toBeTruthy();
});
it("should disable chip button when disabled", () => {
const testApp = fixture.componentInstance;
testApp.disabled.set(true);
fixture.detectChanges();
expect(getChipButton().disabled).toBe(true);
});
it("should accept fullWidth input", () => {
const testApp = fixture.componentInstance;
testApp.fullWidth.set(true);
fixture.detectChanges();
expect(component.fullWidth()).toBe(true);
});
it("should update available options when they change", () => {
const newOptions: ChipSelectOption<string>[] = [
{ label: "New Option 1", value: "new1" },
{ label: "New Option 2", value: "new2" },
];
const testApp = fixture.componentInstance;
testApp.options.set(newOptions);
fixture.detectChanges();
getChipButton().click();
fixture.detectChanges();
const menuItems = Array.from(document.querySelectorAll<HTMLButtonElement>("[bitMenuItem]"));
expect(menuItems.some((el) => el.textContent?.includes("New Option 1"))).toBe(true);
expect(menuItems.some((el) => el.textContent?.includes("New Option 2"))).toBe(true);
});
});
describe("Form Integration Behavior", () => {
it("should display selected option when form control value is set", () => {
component.writeValue("opt1");
fixture.detectChanges();
const button = getChipButton();
expect(button.textContent?.trim()).toContain("Option 1");
});
it("should find and display nested option when form control value is set", () => {
component.writeValue("child1");
fixture.detectChanges();
const button = getChipButton();
expect(button.textContent?.trim()).toContain("Child 1");
});
it("should clear selection when form control value is set to null", () => {
component.writeValue("opt1");
fixture.detectChanges();
expect(getChipButton().textContent).toContain("Option 1");
component.writeValue(null as any);
fixture.detectChanges();
expect(getChipButton().textContent).toContain("Select an option");
});
it("should disable chip when form control is disabled", () => {
expect(getChipButton().disabled).toBe(false);
component.setDisabledState(true);
fixture.detectChanges();
expect(getChipButton().disabled).toBe(true);
});
it("should respect both template and programmatic disabled states", () => {
const testApp = fixture.componentInstance;
testApp.disabled.set(true);
fixture.detectChanges();
expect(getChipButton().disabled).toBe(true);
testApp.disabled.set(false);
component.setDisabledState(true);
fixture.detectChanges();
expect(getChipButton().disabled).toBe(true);
component.setDisabledState(false);
fixture.detectChanges();
expect(getChipButton().disabled).toBe(false);
});
it("should integrate with Angular reactive forms", () => {
const formControl = new FormControl<string>("opt1");
component.registerOnChange((value) => formControl.setValue(value));
component.writeValue(formControl.value);
fixture.detectChanges();
expect(component["selectedOption"]?.value).toBe("opt1");
});
it("should update form value when option is selected", () => {
const onChangeSpy = jest.fn();
component.registerOnChange(onChangeSpy);
component.writeValue("opt2");
component["onChange"]({ label: "Option 2", value: "opt2" });
expect(onChangeSpy).toHaveBeenCalledWith("opt2");
});
});
describe("Menu Behavior", () => {
it("should open menu when chip button is clicked", () => {
const chipButton = getChipButton();
chipButton.click();
fixture.detectChanges();
expect(getBitMenuPanel()).toBeTruthy();
});
it("should close menu when backdrop is clicked", () => {
getChipButton().click();
fixture.detectChanges();
expect(getBitMenuPanel()).toBeTruthy();
const backdrop = document.querySelector(".cdk-overlay-backdrop") as HTMLElement;
backdrop.click();
fixture.detectChanges();
expect(getBitMenuPanel()).toBeFalsy();
});
it("should not open menu when disabled", () => {
const testApp = fixture.componentInstance;
testApp.disabled.set(true);
fixture.detectChanges();
getChipButton().click();
fixture.detectChanges();
expect(getBitMenuPanel()).toBeFalsy();
});
it("should focus first menu item when menu opens", async () => {
const trigger = getMenuTriggerDirective();
trigger.toggleMenu();
fixture.detectChanges();
await fixture.whenStable();
const menu = component.menu();
expect(menu?.keyManager?.activeItemIndex).toBe(0);
});
it("should not focus menu items during initialization (before menu opens)", () => {
const menu = component.menu();
expect(menu?.keyManager?.activeItemIndex).toBe(-1);
});
it("should calculate and set menu width on open", () => {
getChipButton().click();
fixture.detectChanges();
expect(component["menuWidth"]).toBeGreaterThanOrEqual(0);
});
it("should reset menu width when menu closes", () => {
getChipButton().click();
fixture.detectChanges();
component["menuWidth"] = 200;
getMenuTriggerDirective().toggleMenu();
fixture.detectChanges();
expect(component["menuWidth"]).toBeNull();
});
});
describe("Option Selection", () => {
it("should select option and notify form control", () => {
const onChangeSpy = jest.fn();
component.registerOnChange(onChangeSpy);
const option = testOptions[0];
component["selectOption"](option, new MouseEvent("click"));
expect(component["selectedOption"]).toEqual(option);
expect(onChangeSpy).toHaveBeenCalledWith("opt1");
});
it("should display selected option label in chip button", () => {
component.writeValue("opt1");
fixture.detectChanges();
const button = getChipButton();
expect(button.textContent?.trim()).toContain("Option 1");
});
it("should show clear button when option is selected", () => {
expect(getClearButton()).toBeFalsy();
component.writeValue("opt1");
fixture.detectChanges();
expect(getClearButton()).toBeTruthy();
});
it("should clear selection when clear button is clicked", () => {
const onChangeSpy = jest.fn();
component.registerOnChange(onChangeSpy);
component.writeValue("opt1");
fixture.detectChanges();
const clearButton = getClearButton();
clearButton.click();
fixture.detectChanges();
expect(component["selectedOption"]).toBeNull();
expect(onChangeSpy).toHaveBeenCalledWith(null);
});
it("should display placeholder when no option is selected", () => {
expect(component["selectedOption"]).toBeFalsy();
expect(getChipButton().textContent).toContain("Select an option");
});
it("should display option icon when selected", () => {
component.writeValue("opt1");
fixture.detectChanges();
const icon = fixture.debugElement.query(By.css('i[aria-hidden="true"]'));
expect(icon).toBeTruthy();
});
});
describe("Nested Options Navigation", () => {
it("should navigate to child options when parent is clicked", () => {
getChipButton().click();
fixture.detectChanges();
const parentMenuItem = Array.from(
document.querySelectorAll<HTMLButtonElement>("[bitMenuItem]"),
).find((el) => el.textContent?.includes("Parent Option"));
expect(parentMenuItem).toBeTruthy();
parentMenuItem?.click();
fixture.detectChanges();
expect(component["renderedOptions"]?.value).toBe("parent");
});
it("should show back navigation when in submenu", async () => {
component.writeValue("child1");
component["setOrResetRenderedOptions"]();
fixture.detectChanges();
getChipButton().click();
fixture.detectChanges();
await fixture.whenStable();
const backButton = Array.from(
document.querySelectorAll<HTMLButtonElement>("[bitMenuItem]"),
).find((el) => el.textContent?.includes("Back to"));
expect(backButton).toBeTruthy();
});
it("should navigate back to parent menu", async () => {
component.writeValue("child1");
component["setOrResetRenderedOptions"]();
fixture.detectChanges();
getChipButton().click();
fixture.detectChanges();
await fixture.whenStable();
const backButton = Array.from(
document.querySelectorAll<HTMLButtonElement>("[bitMenuItem]"),
).find((el) => el.textContent?.includes("Back to"));
expect(backButton).toBeTruthy();
backButton?.dispatchEvent(new MouseEvent("click"));
fixture.detectChanges();
expect(component["renderedOptions"]?.value).toBeNull();
});
it("should update rendered options when selected option has children", () => {
component.writeValue("parent");
component["setOrResetRenderedOptions"]();
expect(component["renderedOptions"]?.value).toBe("parent");
});
it("should show parent menu if selected option has no children", () => {
component.writeValue("child1");
component["setOrResetRenderedOptions"]();
expect(component["renderedOptions"]?.value).toBe("parent");
});
});
describe("Disabled State Behavior", () => {
it("should disable clear button when disabled", () => {
component.writeValue("opt1");
fixture.detectChanges();
const testApp = fixture.componentInstance;
testApp.disabled.set(true);
fixture.detectChanges();
const clearButton = getClearButton();
expect(clearButton.disabled).toBe(true);
});
});
describe("Focus Management", () => {
it("should track focus-visible-within state", () => {
const chipButton = getChipButton();
chipButton.dispatchEvent(new FocusEvent("focusin", { bubbles: true }));
fixture.detectChanges();
expect(component["focusVisibleWithin"]()).toBe(false);
});
it("should clear focus-visible-within on focusout", () => {
component["focusVisibleWithin"].set(true);
const chipButton = getChipButton();
chipButton.dispatchEvent(new FocusEvent("focusout", { bubbles: true }));
fixture.detectChanges();
expect(component["focusVisibleWithin"]()).toBe(false);
});
});
describe("Edge Cases", () => {
it("should handle empty options array", () => {
const testApp = fixture.componentInstance;
testApp.options.set([]);
fixture.detectChanges();
expect(component.options()).toEqual([]);
expect(component["rootTree"]?.children).toEqual([]);
});
it("should handle options without icons", () => {
const testApp = fixture.componentInstance;
testApp.options.set([{ label: "No Icon Option", value: "no-icon" }]);
fixture.detectChanges();
expect(component.options()).toEqual([{ label: "No Icon Option", value: "no-icon" }]);
});
it("should handle disabled options in menu", () => {
const testApp = fixture.componentInstance;
testApp.options.set([
{ label: "Enabled Option", value: "enabled" },
{ label: "Disabled Option", value: "disabled", disabled: true },
]);
fixture.detectChanges();
getChipButton().click();
fixture.detectChanges();
const disabledMenuItem = Array.from(
document.querySelectorAll<HTMLButtonElement>("[bitMenuItem]"),
).find((el) => el.textContent?.includes("Disabled Option"));
expect(disabledMenuItem?.disabled).toBe(true);
});
});
});
@Component({
selector: "test-app",
template: `
<bit-chip-select
placeholderText="Select an option"
placeholderIcon="bwi-filter"
[options]="options()"
[disabled]="disabled()"
[fullWidth]="fullWidth()"
/>
`,
imports: [ChipSelectComponent],
changeDetection: ChangeDetectionStrategy.OnPush,
})
class TestApp {
readonly options = signal<ChipSelectOption<string>[]>([
{ label: "Option 1", value: "opt1", icon: "bwi-folder" },
{ label: "Option 2", value: "opt2" },
{
label: "Parent Option",
value: "parent",
children: [
{ label: "Child 1", value: "child1" },
{ label: "Child 2", value: "child2" },
],
},
]);
readonly disabled = signal(false);
readonly fullWidth = signal(false);
}

View File

@@ -1,27 +1,26 @@
import {
AfterViewInit,
Component,
DestroyRef,
ElementRef,
HostBinding,
HostListener,
Input,
QueryList,
ViewChildren,
booleanAttribute,
inject,
computed,
effect,
signal,
input,
viewChild,
viewChildren,
ChangeDetectionStrategy,
ChangeDetectorRef,
inject,
} from "@angular/core";
import { takeUntilDestroyed } from "@angular/core/rxjs-interop";
import { ControlValueAccessor, NG_VALUE_ACCESSOR } from "@angular/forms";
import { compareValues } from "@bitwarden/common/platform/misc/compare-values";
import { ButtonModule } from "../button";
import { IconButtonModule } from "../icon-button";
import { MenuComponent, MenuItemDirective, MenuModule } from "../menu";
import { MenuComponent, MenuItemDirective, MenuModule, MenuTriggerForDirective } from "../menu";
import { Option } from "../select/option";
import { SharedModule } from "../shared";
import { TypographyModule } from "../typography";
@@ -35,8 +34,6 @@ export type ChipSelectOption<T> = Option<T> & {
/**
* `<bit-chip-select>` is a select element that is commonly used to filter items in lists or tables.
*/
// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush
// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection
@Component({
selector: "bit-chip-select",
templateUrl: "chip-select.component.html",
@@ -48,13 +45,15 @@ export type ChipSelectOption<T> = Option<T> & {
multi: true,
},
],
changeDetection: ChangeDetectionStrategy.OnPush,
})
export class ChipSelectComponent<T = unknown> implements ControlValueAccessor, AfterViewInit {
export class ChipSelectComponent<T = unknown> implements ControlValueAccessor {
private readonly cdr = inject(ChangeDetectorRef);
readonly menu = viewChild(MenuComponent);
// FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals
// eslint-disable-next-line @angular-eslint/prefer-signals
@ViewChildren(MenuItemDirective) menuItems?: QueryList<MenuItemDirective>;
readonly menuItems = viewChildren(MenuItemDirective);
readonly chipSelectButton = viewChild<ElementRef<HTMLButtonElement>>("chipSelectButton");
readonly menuTrigger = viewChild(MenuTriggerForDirective);
/** Text to show when there is no selected option */
readonly placeholderText = input.required<string>();
@@ -62,28 +61,20 @@ export class ChipSelectComponent<T = unknown> implements ControlValueAccessor, A
/** Icon to show when there is no selected option or the selected option does not have an icon */
readonly placeholderIcon = input<string>();
private _options: ChipSelectOption<T>[] = [];
// TODO: Skipped for signal migration because:
// Accessor inputs cannot be migrated as they are too complex.
/** The select options to render */
// FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals
// eslint-disable-next-line @angular-eslint/prefer-signals
@Input({ required: true })
get options(): ChipSelectOption<T>[] {
return this._options;
}
set options(value: ChipSelectOption<T>[]) {
this._options = value;
this.initializeRootTree(value);
}
readonly options = input.required<ChipSelectOption<T>[]>();
/** Disables the entire chip */
// TODO: Skipped for signal migration because:
// Your application code writes to the input. This prevents migration.
// FIXME(https://bitwarden.atlassian.net/browse/CL-903): Migrate to Signals
// eslint-disable-next-line @angular-eslint/prefer-signals
@Input({ transform: booleanAttribute }) disabled = false;
/** Disables the entire chip (template input) */
protected readonly disabledInput = input<boolean, unknown>(false, {
alias: "disabled",
transform: booleanAttribute,
});
/** Disables the entire chip (programmatic control from CVA) */
private readonly disabledState = signal(false);
/** Combined disabled state from both input and programmatic control */
readonly disabled = computed(() => this.disabledInput() || this.disabledState());
/** Chip will stretch to full width of its container */
readonly fullWidth = input<boolean, unknown>(undefined, { transform: booleanAttribute });
@@ -106,11 +97,30 @@ export class ChipSelectComponent<T = unknown> implements ControlValueAccessor, A
return ["tw-inline-block", this.fullWidth() ? "tw-w-full" : "tw-max-w-52"];
}
private destroyRef = inject(DestroyRef);
/** Tree constructed from `this.options` */
private rootTree?: ChipSelectOption<T> | null;
constructor() {
// Initialize the root tree whenever options change
effect(() => {
this.initializeRootTree(this.options());
});
// Focus the first menu item when menuItems change (e.g., navigating submenus)
effect(() => {
// Trigger effect when menuItems changes
const items = this.menuItems();
const currentMenu = this.menu();
const trigger = this.menuTrigger();
// Note: `isOpen` is intentionally accessed outside signal tracking (via `trigger?.isOpen`)
// to avoid re-focusing when the menu state changes. We only want to focus during
// submenu navigation, not on initial open/close.
if (items.length > 0 && trigger?.isOpen) {
currentMenu?.keyManager?.setFirstItemActive();
}
});
}
/** Options that are currently displayed in the menu */
protected renderedOptions?: ChipSelectOption<T> | null;
@@ -225,16 +235,6 @@ export class ChipSelectComponent<T = unknown> implements ControlValueAccessor, A
this.renderedOptions = this.rootTree;
}
ngAfterViewInit() {
/**
* menuItems will change when the user navigates into or out of a submenu. when that happens, we want to
* direct their focus to the first item in the new menu
*/
this.menuItems?.changes.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => {
this.menu()?.keyManager?.setFirstItemActive();
});
}
/**
* Calculate the width of the menu based on whichever is larger, the chip select width or the width of
* the initially rendered options
@@ -257,6 +257,9 @@ export class ChipSelectComponent<T = unknown> implements ControlValueAccessor, A
writeValue(obj: T): void {
this.selectedOption = this.findOption(this.rootTree, obj);
this.setOrResetRenderedOptions();
// OnPush components require manual change detection when writeValue() is called
// externally by Angular forms, as the framework doesn't automatically trigger CD
this.cdr.markForCheck();
}
/** Implemented as part of NG_VALUE_ACCESSOR */
@@ -271,7 +274,7 @@ export class ChipSelectComponent<T = unknown> implements ControlValueAccessor, A
/** Implemented as part of NG_VALUE_ACCESSOR */
setDisabledState(isDisabled: boolean): void {
this.disabled = isDisabled;
this.disabledState.set(isDisabled);
}
/** Implemented as part of NG_VALUE_ACCESSOR */