From 7e3d112efdf570d0f44ef3de32a773539149a775 Mon Sep 17 00:00:00 2001 From: mimshins Date: Sun, 18 May 2025 20:08:01 +0330 Subject: [PATCH 1/2] refactor to update `value` property to be readonly --- .changeset/sad-sloths-mate.md | 6 + .../src/pinwheel/item/Controller.ts | 16 +- .../web-components/src/pinwheel/item/item.ts | 2 +- .../web-components/src/pinwheel/pinwheel.ts | 149 ++++++++---------- 4 files changed, 74 insertions(+), 99 deletions(-) create mode 100644 .changeset/sad-sloths-mate.md diff --git a/.changeset/sad-sloths-mate.md b/.changeset/sad-sloths-mate.md new file mode 100644 index 00000000..8dc645e7 --- /dev/null +++ b/.changeset/sad-sloths-mate.md @@ -0,0 +1,6 @@ +--- +"@tapsioss/web-components": minor +--- + +Update `value` property to be readonly + \ No newline at end of file diff --git a/packages/web-components/src/pinwheel/item/Controller.ts b/packages/web-components/src/pinwheel/item/Controller.ts index 7101817d..96a1ca4b 100644 --- a/packages/web-components/src/pinwheel/item/Controller.ts +++ b/packages/web-components/src/pinwheel/item/Controller.ts @@ -29,21 +29,6 @@ class ItemSelectionController extends SelectionController { this._host.dispatchEvent(new Event("change", { bubbles: true })); } - public override handleSelectionChange(): void { - super.handleSelectionChange(); - - const parent = this._parentTarget as Pinwheel | null; - - if (!parent) return; - if (parent.disabled) return; - - const selectedItem = this._elements.find(item => item.selected); - - if (!selectedItem) return; - - parent.value = selectedItem.value; - } - public override async handleClick(event: MouseEvent): Promise { const parent = this._parentTarget as Pinwheel | null; @@ -53,6 +38,7 @@ class ItemSelectionController extends SelectionController { if (!(await super.handleClick(event))) return false; this._emitValueChange(); + parent.setViewOnItem(this._host); return true; } diff --git a/packages/web-components/src/pinwheel/item/item.ts b/packages/web-components/src/pinwheel/item/item.ts index ef518566..56a243ec 100644 --- a/packages/web-components/src/pinwheel/item/item.ts +++ b/packages/web-components/src/pinwheel/item/item.ts @@ -30,7 +30,7 @@ export class PinwheelItem extends LitElement { * @attr {string} selected * @default false */ - @property({ type: Boolean, reflect: true }) + @property({ type: Boolean }) public get selected(): boolean { return this._selected; } diff --git a/packages/web-components/src/pinwheel/pinwheel.ts b/packages/web-components/src/pinwheel/pinwheel.ts index ccf68974..5e450955 100644 --- a/packages/web-components/src/pinwheel/pinwheel.ts +++ b/packages/web-components/src/pinwheel/pinwheel.ts @@ -112,32 +112,6 @@ export class Pinwheel extends BaseClass { @property({ type: Boolean }) public override autofocus = false; - /** - * The value of the currently selected item. - * - * @prop {string} value - * @attr {string} value - * @default "" - */ - @property({ attribute: false }) - public get value(): string { - return this._value; - } - - public set value(newValue: string) { - if (newValue === this._value) return; - - this._value = newValue; - - if (isSsr() || !this.isConnected) return; - - if (!this.hasUpdated && newValue !== "") { - void this.updateComplete.then(() => { - this._setSelectedItem(newValue); - }); - } else this._setSelectedItem(newValue); - } - @state() private _spinArias: { valueNow: string; @@ -159,8 +133,6 @@ export class Pinwheel extends BaseClass { @query("#container") private _container!: HTMLElement | null; - private _value: string = ""; - private _isProgrammaticallyScrolling = false; constructor() { @@ -184,6 +156,12 @@ export class Pinwheel extends BaseClass { } } + protected override updated(changed: PropertyValues): void { + super.updated(changed); + + if (!this.value) this._selectFirstItem(); + } + protected override firstUpdated(changed: PropertyValues): void { super.firstUpdated(changed); @@ -191,26 +169,17 @@ export class Pinwheel extends BaseClass { logger(ErrorMessages.USE_VALUE_MIN_AND_VALUE_MAX, scope, "warning"); } + const selectedItem = this.selectedItem; + runAfterRepaint(() => { - if (!this.autofocus) return; + if (this.value && selectedItem) this.setViewOnItem(selectedItem); + if (this.autofocus) this.focus(); - this.focus(); + this._noTransition = false; }); } - protected override updated(changed: PropertyValues): void { - super.updated(changed); - - if (changed.has("value")) { - if (!this.value) this._selectFirstItem(); - - runAfterRepaint(() => { - this.setViewOnItem(this.value); - }); - } - } - - private _getScrollDistance(value = this.value): number { + private _getScrollDistance(value: string): number { // Invalidate cache since this function will only be called programmatically // and we need to get the latest items every time. this._cachedItems = null; @@ -234,10 +203,29 @@ export class Pinwheel extends BaseClass { return dy; } - public setViewOnItem(itemValue: string): void { + /** + * The selected pinwheel item. + */ + public get selectedItem(): PinwheelItem | null { + return this._items.find(item => item.selected) ?? null; + } + + /** + * The value of the pinwheel. + */ + public get value(): string { + return this.selectedItem?.value ?? ""; + } + + /** + * Sets the scroll view on the provided item. + * + * @param item The item to set the view on. + */ + public setViewOnItem(item: PinwheelItem): void { if (!this._container || !this._root) return; - const dy = this._getScrollDistance(itemValue); + const dy = this._getScrollDistance(item.value); if (this._root.scrollTop !== dy) { this._isProgrammaticallyScrolling = true; @@ -256,6 +244,7 @@ export class Pinwheel extends BaseClass { } private get _items() { + if (!this.renderRoot) return []; if (this._cachedItems) return this._cachedItems; const itemsSlot = getRenderRootSlot(this.renderRoot, Slots.DEFAULT); @@ -276,47 +265,28 @@ export class Pinwheel extends BaseClass { // programatically and we need to get the latest items every time. this._cachedItems = null; - const firstItem = this._items[0]; + const items = this._items; + const firstItem = items[0]; if (!firstItem) return; if (firstItem.selected) return; - this._setSelectedItem(firstItem.value); - } - - private _setSelectedItem(itemValue: string) { - // Invalidate cache since this function will only be called - // programatically and we need to get the latest items every time. - this._cachedItems = null; - - const items = this._items; - - const targetItem = items.find(item => { - return item.value === itemValue; - }); - - if (!targetItem) return; - if (targetItem.selected) return; - items.forEach(item => { - if (item !== targetItem) item.selected = false; + if (item !== firstItem) item.selected = false; }); - targetItem.selected = true; + firstItem.selected = true; + this.setViewOnItem(firstItem); this._spinArias = { - valueNow: targetItem.value, - valueText: targetItem.textContent?.trim() ?? "", + valueNow: firstItem.value, + valueText: firstItem.textContent?.trim() ?? "", }; } - private _emitValueChange(newValue: string) { + private _emitValueChange() { if (this.disabled) return; - this.setViewOnItem(newValue); - - this.value = newValue; - this.dispatchEvent(new Event("change", { bubbles: true })); } @@ -353,7 +323,8 @@ export class Pinwheel extends BaseClass { nextItem.selected = true; - this._emitValueChange(nextItem.value); + this.setViewOnItem(nextItem); + this._emitValueChange(); return true; } @@ -371,7 +342,8 @@ export class Pinwheel extends BaseClass { nextItem.selected = true; - this._emitValueChange(nextItem.value); + this.setViewOnItem(nextItem); + this._emitValueChange(); return true; } @@ -386,7 +358,8 @@ export class Pinwheel extends BaseClass { nextItem.selected = true; - this._emitValueChange(nextItem.value); + this.setViewOnItem(nextItem); + this._emitValueChange(); return true; } @@ -401,7 +374,8 @@ export class Pinwheel extends BaseClass { nextItem.selected = true; - this._emitValueChange(nextItem.value); + this.setViewOnItem(nextItem); + this._emitValueChange(); return true; } @@ -417,10 +391,6 @@ export class Pinwheel extends BaseClass { if (this._isProgrammaticallyScrolling) { this._isProgrammaticallyScrolling = false; - const dy = this._getScrollDistance(); - - if (dy === this._root.scrollTop) this._noTransition = false; - return; } @@ -429,7 +399,10 @@ export class Pinwheel extends BaseClass { if (!item) return; - this._emitValueChange(item.value); + item.selected = true; + + this.setViewOnItem(item); + this._emitValueChange(); }, 120); private _getClosestFrameNumber() { @@ -452,6 +425,7 @@ export class Pinwheel extends BaseClass { return 0; } + /** @internal */ public override formDisabledCallback(disabled: boolean): void { this.disabled = disabled; } @@ -463,12 +437,21 @@ export class Pinwheel extends BaseClass { /** @internal */ public override formResetCallback(): void { - this.value = this.getAttribute("value") ?? ""; + this._items.forEach(item => { + item.selected = item.hasAttribute("selected"); + + if (item.selected) this.setViewOnItem(item); + }); } /** @internal */ public override formStateRestoreCallback(state: string): void { - this.value = state; + this._items.forEach(item => { + if (item.value === state) { + item.selected = true; + this.setViewOnItem(item); + } else item.selected = false; + }); } protected override render(): TemplateResult { From e1dbd304e225f21872d4f04382f45f62039ee26d Mon Sep 17 00:00:00 2001 From: mimshins Date: Mon, 19 May 2025 13:28:34 +0330 Subject: [PATCH 2/2] remove caching mechanism for items --- .../web-components/src/pinwheel/item/item.ts | 21 +++++- .../src/pinwheel/pinwheel.test.ts | 64 ++++++++++++++----- .../web-components/src/pinwheel/pinwheel.ts | 22 ++----- 3 files changed, 74 insertions(+), 33 deletions(-) diff --git a/packages/web-components/src/pinwheel/item/item.ts b/packages/web-components/src/pinwheel/item/item.ts index 56a243ec..75b450b8 100644 --- a/packages/web-components/src/pinwheel/item/item.ts +++ b/packages/web-components/src/pinwheel/item/item.ts @@ -4,7 +4,7 @@ import { type CSSResultGroup, type TemplateResult, } from "lit"; -import { property } from "lit/decorators.js"; +import { property, query } from "lit/decorators.js"; import { classMap } from "lit/directives/class-map.js"; import { logger } from "../../utils/index.ts"; import ItemSelectionController from "./Controller.ts"; @@ -56,8 +56,27 @@ export class PinwheelItem extends LitElement { @property() public value = ""; + @query("#root", true) + private _root!: HTMLElement | null; + private readonly _selectionController = new ItemSelectionController(this); + public override connectedCallback(): void { + super.connectedCallback(); + + this.addEventListener("click", this._handleClick); + } + + public override disconnectedCallback(): void { + super.disconnectedCallback(); + + this.removeEventListener("click", this._handleClick); + } + + private _handleClick() { + this._root?.click(); + } + protected override render(): TemplateResult { if (!this.value) { logger( diff --git a/packages/web-components/src/pinwheel/pinwheel.test.ts b/packages/web-components/src/pinwheel/pinwheel.test.ts index 122d4de0..bc054631 100644 --- a/packages/web-components/src/pinwheel/pinwheel.test.ts +++ b/packages/web-components/src/pinwheel/pinwheel.test.ts @@ -5,6 +5,7 @@ import { disposeMocks, expect, render, + setupMocks, test, } from "@internals/test-helpers"; import { ErrorMessages, scope } from "./constants.ts"; @@ -39,7 +40,7 @@ describe("🧩 pinwheel", () => { await expect(component).toBeFocused(); }); - test("🧪 should change active item and trigger events (`activate` and `activechange`) using click", async ({ + test("🧪 should update selected item and trigger events using keyboard", async ({ page, }) => { await render( @@ -53,9 +54,29 @@ describe("🧩 pinwheel", () => { `, ); + const mocks = await setupMocks(page); + const handleChange = mocks.createFakeFn(); + const container = page.getByTestId("test-pinwheel"); + await mocks.events.attachMockedEvent(container, "change", handleChange.ref); + + await page.waitForFunction( + expected => { + const container = document.querySelector("tapsi-pinwheel"); + + if (!container) return false; + + return container.value === expected; + }, + "value-1", + { + timeout: 5000, + }, + ); + await expect(container).toHaveJSProperty("value", "value-1"); + await handleChange.matchResult({ callCount: 0 }); // Focus on pinwheel for keyboard interaction await page.keyboard.press("Tab"); @@ -64,22 +85,27 @@ describe("🧩 pinwheel", () => { // Test arrow keys await page.keyboard.press("ArrowDown"); await expect(container).toHaveJSProperty("value", "value-2"); + await handleChange.matchResult({ callCount: 1 }); await page.keyboard.press("ArrowDown"); await expect(container).toHaveJSProperty("value", "value-3"); + await handleChange.matchResult({ callCount: 2 }); await page.keyboard.press("ArrowUp"); await expect(container).toHaveJSProperty("value", "value-2"); + await handleChange.matchResult({ callCount: 3 }); // Test Home and End keys await page.keyboard.press("Home"); await expect(container).toHaveJSProperty("value", "value-1"); + await handleChange.matchResult({ callCount: 4 }); await page.keyboard.press("End"); await expect(container).toHaveJSProperty("value", "value-3"); + await handleChange.matchResult({ callCount: 5 }); }); - test("🧪 should change active item and trigger events (`activate` and `activechange`) using keyboard navigation", async ({ + test("🧪 should update selected item and trigger events using click", async ({ page, }) => { await render( @@ -94,27 +120,35 @@ describe("🧩 pinwheel", () => { ); const container = page.getByTestId("test-pinwheel"); - const item1 = page.getByTestId("test-pinwheel-item-1"); - const item2 = page.getByTestId("test-pinwheel-item-2"); - const item3 = page.getByTestId("test-pinwheel-item-3"); - - await item1.click(); - await expect(container).toHaveJSProperty("value", "value-1"); + const mocks = await setupMocks(page); + const handleChange = mocks.createFakeFn(); - await item2.click(); - await expect(container).toHaveJSProperty("value", "value-2"); + const click = (itemId: string) => { + return page.getByTestId(itemId).click(); + }; - await item3.click(); - await expect(container).toHaveJSProperty("value", "value-3"); + await mocks.events.attachMockedEvent(container, "change", handleChange.ref); + await handleChange.matchResult({ callCount: 0 }); - await item1.click(); + await click("test-pinwheel-item-1"); await expect(container).toHaveJSProperty("value", "value-1"); + await handleChange.matchResult({ callCount: 0 }); - await item3.click(); + await click("test-pinwheel-item-2"); + await expect(container).toHaveJSProperty("value", "value-2"); + await handleChange.matchResult({ callCount: 1 }); + + await click("test-pinwheel-item-3"); await expect(container).toHaveJSProperty("value", "value-3"); + await handleChange.matchResult({ callCount: 2 }); - await item2.click(); + await click("test-pinwheel-item-2"); await expect(container).toHaveJSProperty("value", "value-2"); + await handleChange.matchResult({ callCount: 3 }); + + await click("test-pinwheel-item-1"); + await expect(container).toHaveJSProperty("value", "value-1"); + await handleChange.matchResult({ callCount: 4 }); }); test("🧪 should throw error if no valid label was set for the input", async ({ diff --git a/packages/web-components/src/pinwheel/pinwheel.ts b/packages/web-components/src/pinwheel/pinwheel.ts index 5e450955..80b0a30e 100644 --- a/packages/web-components/src/pinwheel/pinwheel.ts +++ b/packages/web-components/src/pinwheel/pinwheel.ts @@ -121,16 +121,13 @@ export class Pinwheel extends BaseClass { valueText: "", }; - @state() - private _cachedItems: PinwheelItem[] | null = null; - @state() private _noTransition = true; - @query("#root") + @query("#root", true) private _root!: HTMLElement | null; - @query("#container") + @query("#container", true) private _container!: HTMLElement | null; private _isProgrammaticallyScrolling = false; @@ -159,7 +156,9 @@ export class Pinwheel extends BaseClass { protected override updated(changed: PropertyValues): void { super.updated(changed); - if (!this.value) this._selectFirstItem(); + if (!this.value) { + this._selectFirstItem(); + } } protected override firstUpdated(changed: PropertyValues): void { @@ -180,10 +179,6 @@ export class Pinwheel extends BaseClass { } private _getScrollDistance(value: string): number { - // Invalidate cache since this function will only be called programmatically - // and we need to get the latest items every time. - this._cachedItems = null; - const items = this._items; if (items.length <= 1) return 0; @@ -245,7 +240,6 @@ export class Pinwheel extends BaseClass { private get _items() { if (!this.renderRoot) return []; - if (this._cachedItems) return this._cachedItems; const itemsSlot = getRenderRootSlot(this.renderRoot, Slots.DEFAULT); @@ -255,16 +249,10 @@ export class Pinwheel extends BaseClass { .assignedNodes() .filter(node => node instanceof PinwheelItem); - this._cachedItems = items; - return items; } private _selectFirstItem() { - // Invalidate cache since this function will only be called - // programatically and we need to get the latest items every time. - this._cachedItems = null; - const items = this._items; const firstItem = items[0];