From c5bf804fa8536269f8438ef76c06065a885c32dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 20 Nov 2024 15:01:15 +0100 Subject: [PATCH 1/3] fix selectable mixin so it only selects when the right element is clicked --- packages/uui-base/lib/mixins/SelectableMixin.ts | 6 +++++- .../uui-card-media/lib/uui-card-media.test.ts | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/uui-base/lib/mixins/SelectableMixin.ts b/packages/uui-base/lib/mixins/SelectableMixin.ts index 94c05dde3..ca652f7be 100644 --- a/packages/uui-base/lib/mixins/SelectableMixin.ts +++ b/packages/uui-base/lib/mixins/SelectableMixin.ts @@ -112,7 +112,11 @@ export const SelectableMixin = >( } private _handleClick(e: Event) { - if (e.composedPath().indexOf(this.selectableTarget) !== -1) { + const composePath = e.composedPath(); + if ( + (this._selectable || (this.deselectable && this.selected)) && + composePath.indexOf(this.selectableTarget) === 0 + ) { this._toggleSelect(); } } diff --git a/packages/uui-card-media/lib/uui-card-media.test.ts b/packages/uui-card-media/lib/uui-card-media.test.ts index 08b577422..33de9337d 100644 --- a/packages/uui-card-media/lib/uui-card-media.test.ts +++ b/packages/uui-card-media/lib/uui-card-media.test.ts @@ -99,6 +99,22 @@ describe('UUICardMediaElement', () => { expect(event.type).to.equal(UUISelectableEvent.SELECTED); expect(element.selected).to.be.true; }); + it('do react to a click event on performed in this way', async () => { + element.selectable = true; + await elementUpdated(element); + element.click(); + await Promise.resolve(); + expect(element.selected).to.be.true; + }); + it('do not react to a click event on other parts', async () => { + element.selectable = true; + await elementUpdated(element); + element + .shadowRoot!.querySelector('#open-part')! + .click(); + await Promise.resolve(); + expect(element.selected).to.be.false; + }); it('can be selected with keyboard', async () => { element.selectable = true; await elementUpdated(element); From 8d99634382f7d32b9eb945f2e8905a2f65571096 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20Lyngs=C3=B8?= Date: Wed, 20 Nov 2024 15:18:34 +0100 Subject: [PATCH 2/3] further click interaction correction --- .../uui-base/lib/mixins/SelectableMixin.ts | 52 +++++++++---------- .../uui-card-media/lib/uui-card-media.test.ts | 13 +++++ 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/packages/uui-base/lib/mixins/SelectableMixin.ts b/packages/uui-base/lib/mixins/SelectableMixin.ts index ca652f7be..a6dac5ab2 100644 --- a/packages/uui-base/lib/mixins/SelectableMixin.ts +++ b/packages/uui-base/lib/mixins/SelectableMixin.ts @@ -75,11 +75,11 @@ export const SelectableMixin = >( constructor(...args: any[]) { super(...args); - this.addEventListener('click', this._handleClick); - this.addEventListener('keydown', this.handleSelectKeydown); + this.addEventListener('click', this.#onClick); + this.addEventListener('keydown', this.#onKeydown); } - private handleSelectKeydown = (e: KeyboardEvent) => { + #onKeydown = (e: KeyboardEvent) => { const composePath = e.composedPath(); if ( (this._selectable || (this.deselectable && this.selected)) && @@ -87,13 +87,33 @@ export const SelectableMixin = >( ) { if (this.selectableTarget === this) { if (e.code !== 'Space' && e.code !== 'Enter') return; - this._toggleSelect(); + this.#toggleSelect(); e.preventDefault(); } } }; - private _select() { + #onClick = (e: Event) => { + const composePath = e.composedPath(); + if ( + (this._selectable || (this.deselectable && this.selected)) && + composePath.indexOf(this.selectableTarget) === 0 + ) { + this.#toggleSelect(); + } + }; + + #toggleSelect() { + // Only allow for select-interaction if selectable is true. Deselectable is ignored in this case, we do not want a DX where only deselection is a possibility.. + if (!this.selectable) return; + if (this.deselectable === false) { + this.#select(); + } else { + this.selected ? this.#deselect() : this.#select(); + } + } + + #select() { if (!this.selectable) return; const selectEvent = new UUISelectableEvent(UUISelectableEvent.SELECTED); this.dispatchEvent(selectEvent); @@ -102,7 +122,7 @@ export const SelectableMixin = >( this.selected = true; } - private _deselect() { + #deselect() { if (!this.deselectable) return; const selectEvent = new UUISelectableEvent(UUISelectableEvent.DESELECTED); this.dispatchEvent(selectEvent); @@ -110,26 +130,6 @@ export const SelectableMixin = >( this.selected = false; } - - private _handleClick(e: Event) { - const composePath = e.composedPath(); - if ( - (this._selectable || (this.deselectable && this.selected)) && - composePath.indexOf(this.selectableTarget) === 0 - ) { - this._toggleSelect(); - } - } - - private _toggleSelect() { - // Only allow for select-interaction if selectable is true. Deselectable is ignored in this case, we do not want a DX where only deselection is a possibility.. - if (!this.selectable) return; - if (this.deselectable === false) { - this._select(); - } else { - this.selected ? this._deselect() : this._select(); - } - } } // prettier-ignore return (SelectableMixinClass as unknown) as Constructor & T; diff --git a/packages/uui-card-media/lib/uui-card-media.test.ts b/packages/uui-card-media/lib/uui-card-media.test.ts index 33de9337d..6d55dda48 100644 --- a/packages/uui-card-media/lib/uui-card-media.test.ts +++ b/packages/uui-card-media/lib/uui-card-media.test.ts @@ -109,6 +109,19 @@ describe('UUICardMediaElement', () => { it('do not react to a click event on other parts', async () => { element.selectable = true; await elementUpdated(element); + const listener = oneEvent(element, UUICardEvent.OPEN); + element + .shadowRoot!.querySelector('#open-part')! + .click(); + const event = await listener; + expect(event).to.exist; + expect(event.type).to.equal(UUICardEvent.OPEN); + expect(element.selected).to.be.false; + }); + it('do not react to a click event on other parts as href', async () => { + element.selectable = true; + element.href = '#hello'; + await elementUpdated(element); element .shadowRoot!.querySelector('#open-part')! .click(); From 02a3a016519bde4ce4aeea6bd6537b2dd8b5fe16 Mon Sep 17 00:00:00 2001 From: leekelleher Date: Mon, 25 Nov 2024 09:46:16 +0000 Subject: [PATCH 3/3] Marked members as `readonly` Fixes @sonarcloud warnings --- packages/uui-base/lib/mixins/SelectableMixin.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/uui-base/lib/mixins/SelectableMixin.ts b/packages/uui-base/lib/mixins/SelectableMixin.ts index a6dac5ab2..af40771f9 100644 --- a/packages/uui-base/lib/mixins/SelectableMixin.ts +++ b/packages/uui-base/lib/mixins/SelectableMixin.ts @@ -79,7 +79,7 @@ export const SelectableMixin = >( this.addEventListener('keydown', this.#onKeydown); } - #onKeydown = (e: KeyboardEvent) => { + readonly #onKeydown = (e: KeyboardEvent) => { const composePath = e.composedPath(); if ( (this._selectable || (this.deselectable && this.selected)) && @@ -93,7 +93,7 @@ export const SelectableMixin = >( } }; - #onClick = (e: Event) => { + readonly #onClick = (e: Event) => { const composePath = e.composedPath(); if ( (this._selectable || (this.deselectable && this.selected)) &&