From 3ec0d30965be56512d7d86a5b134581ff1b692d4 Mon Sep 17 00:00:00 2001 From: Ken Sternberg Date: Wed, 15 Jan 2025 16:20:12 -0800 Subject: [PATCH] web: restructure ak-dual-select to avoid using CustomEvent # What - Replace all `CustomEvent` handlers with a child class Event. - Use the standard `dispatchEvent()` and `addEventListener()` clauses. - Move common methods of the two panes into an abstract parent class. # Why CustomEvent is a bit of a side-show in JavaScript; it's perfectly acceptable to inherit from Event, and there's much better support for type management while using it, plus deconstructing the received event object is cleaner with child Events than unpacking the `details` object. This codebase is now more open to change, and is closer to our still-evolving style. Doing it this way mean that, once you have an event defined, you don't have to remember if you're sending a custom event or a normal event, and you don't need all that infrastructure for making your Lit objects sensitive to custom event handling. There's enough mixin clutter already. And just like I hate clutter, I hate duplication. The two panes had a lot in common with ARIA handling and storing, clearing, and assigning selected items to the "pending move" table. Moving all that into a parent class exposed the differences: one is a source and the other a sink; one reflects changes made, the other possible changes to be made. # Testing The Storybook tests have all been updated to match the codebase, and there are standalone tests for the various components (pagination, pane control, search) that can be exercised before deployment. --- .../ak-dual-select/ak-dual-select-provider.ts | 36 +++--- .../elements/ak-dual-select/ak-dual-select.ts | 113 ++++++------------ .../ak-dual-select-available-pane.ts | 80 +++---------- .../components/ak-dual-select-controls.ts | 32 ++--- .../components/ak-dual-select-pane.ts | 75 ++++++++++++ .../ak-dual-select-selected-pane.ts | 81 +++---------- .../components/ak-pagination.ts | 6 +- .../components/ak-search-bar.ts | 10 +- web/src/elements/ak-dual-select/events.ts | 112 +++++++++++++++++ .../ak-dual-select-available-pane.stories.ts | 7 +- .../ak-dual-select-controls.stories.ts | 8 +- .../stories/ak-dual-select-master.stories.ts | 7 +- .../ak-dual-select-selected-pane.stories.ts | 7 +- .../stories/ak-pagination.stories.ts | 7 +- web/src/elements/ak-dual-select/types.ts | 7 -- 15 files changed, 310 insertions(+), 278 deletions(-) create mode 100644 web/src/elements/ak-dual-select/components/ak-dual-select-pane.ts create mode 100644 web/src/elements/ak-dual-select/events.ts diff --git a/web/src/elements/ak-dual-select/ak-dual-select-provider.ts b/web/src/elements/ak-dual-select/ak-dual-select-provider.ts index 1a431cca7c..0e024991ee 100644 --- a/web/src/elements/ak-dual-select/ak-dual-select-provider.ts +++ b/web/src/elements/ak-dual-select/ak-dual-select-provider.ts @@ -1,6 +1,5 @@ import { AkControlElement } from "@goauthentik/elements/AkControlElement.js"; import { debounce } from "@goauthentik/elements/utils/debounce"; -import { CustomListenerElement } from "@goauthentik/elements/utils/eventEmitter"; import { msg } from "@lit/localize"; import { PropertyValues, html } from "lit"; @@ -12,6 +11,11 @@ import type { Pagination } from "@goauthentik/api"; import "./ak-dual-select"; import { AkDualSelect } from "./ak-dual-select"; +import { + DualSelectChangeEvent, + DualSelectPaginatorNavEvent, + DualSelectSearchEvent, +} from "./events"; import type { DataProvider, DualSelectPair } from "./types"; /** @@ -26,7 +30,7 @@ import type { DataProvider, DualSelectPair } from "./types"; */ @customElement("ak-dual-select-provider") -export class AkDualSelectProvider extends CustomListenerElement(AkControlElement) { +export class AkDualSelectProvider extends AkControlElement { /** A function that takes a page and returns the DualSelectPair[] collection with which to update * the "Available" pane. * @@ -86,9 +90,9 @@ export class AkDualSelectProvider extends CustomListenerElement(AkControlElement this.onNav = this.onNav.bind(this); this.onChange = this.onChange.bind(this); this.onSearch = this.onSearch.bind(this); - this.addCustomListener("ak-pagination-nav-to", this.onNav); - this.addCustomListener("ak-dual-select-change", this.onChange); - this.addCustomListener("ak-dual-select-search", this.onSearch); + this.addEventListener(DualSelectPaginatorNavEvent.eventName, this.onNav); + this.addEventListener(DualSelectSearchEvent.eventName, this.onSearch); + this.addEventListener(DualSelectChangeEvent.eventName, this.onChange); } willUpdate(changedProperties: PropertyValues) { @@ -122,26 +126,16 @@ export class AkDualSelectProvider extends CustomListenerElement(AkControlElement this.isLoading = false; } - onNav(event: Event) { - if (!(event instanceof CustomEvent)) { - throw new Error(`Expecting a CustomEvent for navigation, received ${event} instead`); - } - this.fetch(event.detail); + onNav(event: DualSelectPaginatorNavEvent) { + this.fetch(event.page); } - onChange(event: Event) { - if (!(event instanceof CustomEvent)) { - throw new Error(`Expecting a CustomEvent for change, received ${event} instead`); - } - this.internalSelected = event.detail.value; - this.selected = this.internalSelected; + onChange(event: DualSelectChangeEvent) { + this.selected = this.internalSelected = event.selected; } - onSearch(event: Event) { - if (!(event instanceof CustomEvent)) { - throw new Error(`Expecting a CustomEvent for change, received ${event} instead`); - } - this.doSearch(event.detail); + onSearch(event: DualSelectSearchEvent) { + this.doSearch(event.search); } doSearch(search: string) { diff --git a/web/src/elements/ak-dual-select/ak-dual-select.ts b/web/src/elements/ak-dual-select/ak-dual-select.ts index 3316129622..129607465d 100644 --- a/web/src/elements/ak-dual-select/ak-dual-select.ts +++ b/web/src/elements/ak-dual-select/ak-dual-select.ts @@ -1,8 +1,5 @@ import { AKElement } from "@goauthentik/elements/Base"; -import { - CustomEmitterElement, - CustomListenerElement, -} from "@goauthentik/elements/utils/eventEmitter"; +import { match } from "ts-pattern"; import { msg, str } from "@lit/localize"; import { PropertyValues, html, nothing } from "lit"; @@ -23,15 +20,13 @@ import { AkDualSelectSelectedPane } from "./components/ak-dual-select-selected-p import "./components/ak-pagination"; import "./components/ak-search-bar"; import { - EVENT_ADD_ALL, - EVENT_ADD_ONE, - EVENT_ADD_SELECTED, - EVENT_DELETE_ALL, - EVENT_REMOVE_ALL, - EVENT_REMOVE_ONE, - EVENT_REMOVE_SELECTED, -} from "./constants"; -import type { BasePagination, DualSelectPair, SearchbarEvent } from "./types"; + DualSelectChangeEvent, + DualSelectMoveRequestEvent, + DualSelectPanelSearchEvent, + DualSelectSearchEvent, + DualSelectUpdateEvent, +} from "./events"; +import type { BasePagination, DualSelectPair } from "./types"; function alphaSort([_k1, v1, s1]: DualSelectPair, [_k2, v2, s2]: DualSelectPair) { const [l, r] = [s1 !== undefined ? s1 : v1, s2 !== undefined ? s2 : v2]; @@ -60,7 +55,7 @@ const keyfinder = k === key; @customElement("ak-dual-select") -export class AkDualSelect extends CustomEmitterElement(CustomListenerElement(AKElement)) { +export class AkDualSelect extends AKElement { static get styles() { return styles; } @@ -96,21 +91,9 @@ export class AkDualSelect extends CustomEmitterElement(CustomListenerElement(AKE super(); this.handleMove = this.handleMove.bind(this); this.handleSearch = this.handleSearch.bind(this); - [ - EVENT_ADD_ALL, - EVENT_ADD_SELECTED, - EVENT_DELETE_ALL, - EVENT_REMOVE_ALL, - EVENT_REMOVE_SELECTED, - EVENT_ADD_ONE, - EVENT_REMOVE_ONE, - ].forEach((eventName: string) => { - this.addCustomListener(eventName, (event: Event) => this.handleMove(eventName, event)); - }); - this.addCustomListener("ak-dual-select-move", () => { - this.requestUpdate(); - }); - this.addCustomListener("ak-search", this.handleSearch); + this.addEventListener(DualSelectMoveRequestEvent.eventName, this.handleMove); + this.addEventListener(DualSelectUpdateEvent.eventName, () => this.requestUpdate()); + this.addEventListener(DualSelectPanelSearchEvent.eventName, this.handleSearch); } willUpdate(changedProperties: PropertyValues) { @@ -123,47 +106,17 @@ export class AkDualSelect extends CustomEmitterElement(CustomListenerElement(AKE } } - handleMove(eventName: string, event: Event) { - if (!(event instanceof CustomEvent)) { - throw new Error(`Expected move event here, got ${eventName}`); - } - - switch (eventName) { - case EVENT_ADD_SELECTED: { - this.addSelected(); - break; - } - case EVENT_REMOVE_SELECTED: { - this.removeSelected(); - break; - } - case EVENT_ADD_ALL: { - this.addAllVisible(); - break; - } - case EVENT_REMOVE_ALL: { - this.removeAllVisible(); - break; - } - case EVENT_DELETE_ALL: { - this.removeAll(); - break; - } - case EVENT_ADD_ONE: { - this.addOne(event.detail); - break; - } - case EVENT_REMOVE_ONE: { - this.removeOne(event.detail); - break; - } - - default: - throw new Error( - `AkDualSelect.handleMove received unknown event type: ${eventName}`, - ); - } - this.dispatchCustomEvent("ak-dual-select-change", { value: this.value }); + handleMove(event: DualSelectMoveRequestEvent) { + match(event.move) + .with("add-all", () => this.addAllVisible()) + .with("add-one", () => this.addOne(event.key)) + .with("add-selected", () => this.addSelected()) + .with("delete-all", () => this.removeAll()) + .with("remove-all", () => this.removeAllVisible()) + .with("remove-one", () => this.removeOne(event.key)) + .with("remove-selected", () => this.removeSelected()) + .exhaustive(); + this.dispatchEvent(new DualSelectChangeEvent(this.value)); event.stopPropagation(); } @@ -182,7 +135,10 @@ export class AkDualSelect extends CustomEmitterElement(CustomListenerElement(AKE this.availablePane.value!.clearMove(); } - addOne(key: string) { + addOne(key?: string) { + if (!key) { + return; + } const requested = this.options.find(keyfinder(key)); if (requested && !this.selected.find(keyfinder(requested[0]))) { this.selected = [...this.selected, requested]; @@ -207,7 +163,10 @@ export class AkDualSelect extends CustomEmitterElement(CustomListenerElement(AKE this.selectedPane.value!.clearMove(); } - removeOne(key: string) { + removeOne(key?: string) { + if (!key) { + return; + } this.selected = this.selected.filter(([k]) => k !== key); } @@ -223,18 +182,18 @@ export class AkDualSelect extends CustomEmitterElement(CustomListenerElement(AKE this.selectedPane.value!.clearMove(); } - handleSearch(event: SearchbarEvent) { - switch (event.detail.source) { + handleSearch(event: DualSelectPanelSearchEvent) { + switch (event.source) { case "ak-dual-list-available-search": - return this.handleAvailableSearch(event.detail.value); + return this.handleAvailableSearch(event.filterOn); case "ak-dual-list-selected-search": - return this.handleSelectedSearch(event.detail.value); + return this.handleSelectedSearch(event.filterOn); } event.stopPropagation(); } handleAvailableSearch(value: string) { - this.dispatchCustomEvent("ak-dual-select-search", value); + this.dispatchEvent(new DualSelectSearchEvent(value)); } handleSelectedSearch(value: string) { diff --git a/web/src/elements/ak-dual-select/components/ak-dual-select-available-pane.ts b/web/src/elements/ak-dual-select/components/ak-dual-select-available-pane.ts index 1d4d235c6d..66ce3607c0 100644 --- a/web/src/elements/ak-dual-select/components/ak-dual-select-available-pane.ts +++ b/web/src/elements/ak-dual-select/components/ak-dual-select-available-pane.ts @@ -1,26 +1,19 @@ -import { AKElement } from "@goauthentik/elements/Base"; -import { CustomEmitterElement } from "@goauthentik/elements/utils/eventEmitter"; +import { bound } from "@goauthentik/elements/decorators/bound"; import { html, nothing } from "lit"; -import { customElement, property, state } from "lit/decorators.js"; +import { customElement, property } from "lit/decorators.js"; import { classMap } from "lit/directives/class-map.js"; import { map } from "lit/directives/map.js"; -import { availablePaneStyles, listStyles } from "./styles.css"; -import PFButton from "@patternfly/patternfly/components/Button/button.css"; -import PFDualListSelector from "@patternfly/patternfly/components/DualListSelector/dual-list-selector.css"; -import PFBase from "@patternfly/patternfly/patternfly-base.css"; +import { availablePaneStyles } from "./styles.css"; -import { EVENT_ADD_ONE } from "../constants"; +import { + DualSelectMoveAvailableEvent, + DualSelectMoveRequestEvent, + DualSelectUpdateEvent, +} from "../events"; import type { DualSelectPair } from "../types"; - -const styles = [PFBase, PFButton, PFDualListSelector, listStyles, availablePaneStyles]; - -const hostAttributes = [ - ["aria-labelledby", "dual-list-selector-available-pane-status"], - ["aria-multiselectable", "true"], - ["role", "listbox"], -]; +import { AkDualSelectAbstractPane } from "./ak-dual-select-pane"; /** * @element ak-dual-select-available-panel @@ -40,9 +33,9 @@ const hostAttributes = [ * */ @customElement("ak-dual-select-available-pane") -export class AkDualSelectAvailablePane extends CustomEmitterElement(AKElement) { +export class AkDualSelectAvailablePane extends AkDualSelectAbstractPane { static get styles() { - return styles; + return [...AkDualSelectAbstractPane.styles, availablePaneStyles]; } /* The array of key/value pairs this pane is currently showing */ @@ -56,68 +49,31 @@ export class AkDualSelectAvailablePane extends CustomEmitterElement(AKElement) { @property({ type: Object }) readonly selected: Set = new Set(); - /* This is the only mutator for this object. It collects the list of objects the user has - * clicked on *in this pane*. It is explicitly marked as "public" to emphasize that the parent - * orchestrator for the dual-select widget can and will access it to get the list of keys to be - * moved (removed) if the user so requests. - * - */ - @state() - public toMove: Set = new Set(); - - constructor() { - super(); - this.onClick = this.onClick.bind(this); - this.onMove = this.onMove.bind(this); - } - - connectedCallback() { - super.connectedCallback(); - hostAttributes.forEach(([attr, value]) => { - if (!this.hasAttribute(attr)) { - this.setAttribute(attr, value); - } - }); - } - - clearMove() { - this.toMove = new Set(); - } - + @bound onClick(key: string) { if (this.selected.has(key)) { return; } - if (this.toMove.has(key)) { - this.toMove.delete(key); - } else { - this.toMove.add(key); - } - this.dispatchCustomEvent( - "ak-dual-select-available-move-changed", - Array.from(this.toMove.values()).sort(), - ); - this.dispatchCustomEvent("ak-dual-select-move"); + this.move(key); + this.dispatchEvent(new DualSelectMoveAvailableEvent(this.moveable.sort())); + this.dispatchEvent(new DualSelectUpdateEvent()); // Necessary because updating a map won't trigger a state change this.requestUpdate(); } + @bound onMove(key: string) { this.toMove.delete(key); - this.dispatchCustomEvent(EVENT_ADD_ONE, key); + this.dispatchEvent(new DualSelectMoveRequestEvent("add-one", key)); this.requestUpdate(); } - get moveable() { - return Array.from(this.toMove.values()); - } - // DO NOT use `Array.map()` instead of Lit's `map()` function. Lit's `map()` is object-aware and // will not re-arrange or reconstruct the list automatically if the actual sources do not // change; this allows the available pane to illustrate selected items with the checkmark // without causing the list to scroll back up to the top. - render() { + override render() { return html`
    diff --git a/web/src/elements/ak-dual-select/components/ak-dual-select-controls.ts b/web/src/elements/ak-dual-select/components/ak-dual-select-controls.ts index b00dce7df2..e8bc6d1eb6 100644 --- a/web/src/elements/ak-dual-select/components/ak-dual-select-controls.ts +++ b/web/src/elements/ak-dual-select/components/ak-dual-select-controls.ts @@ -1,5 +1,4 @@ import { AKElement } from "@goauthentik/elements/Base"; -import { CustomEmitterElement } from "@goauthentik/elements/utils/eventEmitter"; import { msg } from "@lit/localize"; import { css, html, nothing } from "lit"; @@ -8,13 +7,7 @@ import { customElement, property } from "lit/decorators.js"; import PFButton from "@patternfly/patternfly/components/Button/button.css"; import PFBase from "@patternfly/patternfly/patternfly-base.css"; -import { - EVENT_ADD_ALL, - EVENT_ADD_SELECTED, - EVENT_DELETE_ALL, - EVENT_REMOVE_ALL, - EVENT_REMOVE_SELECTED, -} from "../constants"; +import { DualSelectMoveRequestEvent, type MoveEventType } from "../events"; const styles = [ PFBase, @@ -47,7 +40,7 @@ const styles = [ */ @customElement("ak-dual-select-controls") -export class AkDualSelectControls extends CustomEmitterElement(AKElement) { +export class AkDualSelectControls extends AKElement { static get styles() { return styles; } @@ -96,11 +89,11 @@ export class AkDualSelectControls extends CustomEmitterElement(AKElement) { this.onClick = this.onClick.bind(this); } - onClick(eventName: string) { - this.dispatchCustomEvent(eventName); + onClick(eventName: MoveEventType) { + this.dispatchEvent(new DualSelectMoveRequestEvent(eventName)); } - renderButton(label: string, event: string, active: boolean, direction: string) { + renderButton(label: string, event: MoveEventType, active: boolean, direction: string) { return html`
    `; // eslint-disable-next-line @typescript-eslint/no-explicit-any -const handleMoveChanged = (result: any) => { +const handleMoveChanged = (result: DualSelectMoveSelectedEvent) => { const target = document.querySelector("#action-button-message-pad"); target!.innerHTML = ""; - result.detail.forEach((key: string) => { + result.keys.forEach((key: string) => { target!.append(new DOMParser().parseFromString(`
  • ${key}
    • `, "text/xml").firstChild!); }); }; -window.addEventListener("ak-dual-select-selected-move-changed", handleMoveChanged); +window.addEventListener(DualSelectMoveSelectedEvent.eventName, handleMoveChanged); type Story = StoryObj; diff --git a/web/src/elements/ak-dual-select/stories/ak-pagination.stories.ts b/web/src/elements/ak-dual-select/stories/ak-pagination.stories.ts index 0ecbe68af9..0340aeaaa4 100644 --- a/web/src/elements/ak-dual-select/stories/ak-pagination.stories.ts +++ b/web/src/elements/ak-dual-select/stories/ak-pagination.stories.ts @@ -5,6 +5,7 @@ import { TemplateResult, html } from "lit"; import "../components/ak-pagination"; import { AkPagination } from "../components/ak-pagination"; +import { DualSelectPaginatorNavEvent } from "../events"; const metadata: Meta = { title: "Elements / Dual Select / Pagination Control", @@ -43,18 +44,18 @@ const container = (testItem: TemplateResult) =>
`; // eslint-disable-next-line @typescript-eslint/no-explicit-any -const handleMoveChanged = (result: any) => { +const handleMoveChanged = (result: DualSelectPaginatorNavEvent) => { console.debug(result); const target = document.querySelector("#action-button-message-pad"); target!.append( new DOMParser().parseFromString( - `
  • Request to move to page ${result.detail}
    • `, + `
  • Request to move to page ${result.page}
    • `, "text/xml", ).firstChild!, ); }; -window.addEventListener("ak-pagination-nav-to", handleMoveChanged); +window.addEventListener(DualSelectPaginatorNavEvent.eventName, handleMoveChanged); type Story = StoryObj; diff --git a/web/src/elements/ak-dual-select/types.ts b/web/src/elements/ak-dual-select/types.ts index e10d16296d..f903d17824 100644 --- a/web/src/elements/ak-dual-select/types.ts +++ b/web/src/elements/ak-dual-select/types.ts @@ -29,10 +29,3 @@ export type DataProvision = { }; export type DataProvider = (page: number, search?: string) => Promise; - -export interface SearchbarEvent extends CustomEvent { - detail: { - source: string; - value: string; - }; -}