From 7ccbe2e90d240438eef7085e822c15e3d02f0436 Mon Sep 17 00:00:00 2001 From: Felix Mueller Date: Tue, 27 Jul 2021 19:25:40 +0200 Subject: [PATCH 1/4] fix(input-amount): Reflect more currency changes Switching between defined / undefined currency yields better currency label updates Closes #1430 --- packages/input-amount/src/LionInputAmount.js | 79 ++++++++++++------- .../test/lion-input-amount.test.js | 33 ++++++++ 2 files changed, 82 insertions(+), 30 deletions(-) diff --git a/packages/input-amount/src/LionInputAmount.js b/packages/input-amount/src/LionInputAmount.js index d09fd3da7..ebe130de8 100644 --- a/packages/input-amount/src/LionInputAmount.js +++ b/packages/input-amount/src/LionInputAmount.js @@ -30,28 +30,6 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { }; } - get slots() { - return { - ...super.slots, - after: () => { - if (this.currency) { - const el = document.createElement('span'); - // The data-label attribute will make sure that FormControl adds this to - // input[aria-labelledby] - el.setAttribute('data-label', ''); - - el.textContent = this.__currencyLabel; - return el; - } - return undefined; - }, - }; - } - - get _currencyDisplayNode() { - return Array.from(this.children).find(child => child.slot === 'after'); - } - static get styles() { return [ ...super.styles, @@ -88,8 +66,8 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { /** @param {import('@lion/core').PropertyValues } changedProperties */ updated(changedProperties) { super.updated(changedProperties); - if (changedProperties.has('currency') && this.currency) { - this._onCurrencyChanged({ currency: this.currency }); + if (changedProperties.has('currency')) { + this._onCurrencyChanged({ currency: this.currency || null }); } if (changedProperties.has('locale') && this.locale !== changedProperties.get('locale')) { @@ -126,25 +104,66 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { /** * @param {Object} opts - * @param {string} opts.currency + * @param {string?} opts.currency * @protected */ _onCurrencyChanged({ currency }) { - if (this._isPrivateSlot('after') && this._currencyDisplayNode) { + this.formatOptions.currency = currency || undefined; + if (this.currency) { + if (!this._currencyDisplayNode) { + this._currencyDisplayNode = this._createCurrencyDisplayNode(); + } this._currencyDisplayNode.textContent = this.__currencyLabel; + this._calculateValues({ source: null }); + } else { + this._currencyDisplayNode = undefined; } - this.formatOptions.currency = currency; - this._calculateValues({ source: null }); this.__setCurrencyDisplayLabel(); } + /** + * @returns the current currency display node + * @protected + */ + get _currencyDisplayNode() { + return Array.from(this.children).find(child => child.slot === 'after'); + } + + /** + * @protected + */ + set _currencyDisplayNode(node) { + if (node) { + this.appendChild(node); + } else { + this._currencyDisplayNode?.remove(); + } + } + + /** + * @returns a newly created node for displaying the currency + * @protected + */ + _createCurrencyDisplayNode() { + const el = document.createElement('span'); + // The data-label attribute will make sure that FormControl adds this to + // input[aria-labelledby] + el.setAttribute('data-label', ''); + el.textContent = this.__currencyLabel; + el.slot = 'after'; + return el; + } + /** @private */ __setCurrencyDisplayLabel() { // TODO: (@erikkroes) for optimal a11y, abbreviations should be part of aria-label // example, for a language switch with text 'en', an aria-label of 'english' is not // sufficient, it should also contain the abbreviation. - if (this.currency && this._currencyDisplayNode) { - this._currencyDisplayNode.setAttribute('aria-label', getCurrencyName(this.currency, {})); + if (this._currencyDisplayNode) { + this._currencyDisplayNode.setAttribute( + 'aria-label', + this.currency ? getCurrencyName(this.currency, {}) : '', + ); } } diff --git a/packages/input-amount/test/lion-input-amount.test.js b/packages/input-amount/test/lion-input-amount.test.js index 14cb160b4..ebfd3c7b3 100644 --- a/packages/input-amount/test/lion-input-amount.test.js +++ b/packages/input-amount/test/lion-input-amount.test.js @@ -134,6 +134,7 @@ describe('', () => { const el = /** @type {LionInputAmount} */ ( await fixture(``) ); + expect( /** @type {HTMLElement[]} */ (Array.from(el.children)).find(child => child.slot === 'after') ?.innerText, @@ -197,6 +198,38 @@ describe('', () => { expect(el.formattedValue).to.equal('123,45'); }); + it('removes the currency label when currency switches from EUR to undefined', async () => { + const el = /** @type {LionInputAmount} */ ( + await fixture(``) + ); + expect( + /** @type {HTMLElement[]} */ (Array.from(el.children)).find(child => child.slot === 'after') + ?.innerText, + ).to.equal('EUR'); + el.currency = undefined; + await el.updateComplete; + expect( + /** @type {HTMLElement[]} */ (Array.from(el.children)).find(child => child.slot === 'after'), + ).to.be.undefined; + }); + + it('adds the currency label when currency switches from undefined to EUR', async () => { + const el = /** @type {LionInputAmount} */ ( + await fixture(``) + ); + expect( + /** @type {HTMLElement[]} */ (Array.from(el.children)).find(child => child.slot === 'after'), + ).to.be.undefined; + + el.currency = 'EUR'; + await el.updateComplete; + const currLabel = /** @type {HTMLElement[]} */ (Array.from(el.children)).find( + child => child.slot === 'after', + ); + expect(currLabel?.innerText).to.equal('EUR'); + expect(currLabel?.getAttribute('aria-label')).to.equal('euros'); + }); + describe('Accessibility', () => { it('is accessible', async () => { const el = await fixture( From 99d894efb3926b4c7894854d0bc35231c2a2685b Mon Sep 17 00:00:00 2001 From: Felix Mueller Date: Thu, 29 Jul 2021 07:26:05 +0200 Subject: [PATCH 2/4] Make Currency Display Node Private --- packages/input-amount/src/LionInputAmount.js | 50 +++++++++---------- .../test/lion-input-amount.test.js | 14 ++++-- 2 files changed, 35 insertions(+), 29 deletions(-) diff --git a/packages/input-amount/src/LionInputAmount.js b/packages/input-amount/src/LionInputAmount.js index ebe130de8..a2a62efca 100644 --- a/packages/input-amount/src/LionInputAmount.js +++ b/packages/input-amount/src/LionInputAmount.js @@ -110,36 +110,17 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { _onCurrencyChanged({ currency }) { this.formatOptions.currency = currency || undefined; if (this.currency) { - if (!this._currencyDisplayNode) { - this._currencyDisplayNode = this._createCurrencyDisplayNode(); + if (!this.__currencyDisplayNode) { + this.__currencyDisplayNode = this._createCurrencyDisplayNode(); } - this._currencyDisplayNode.textContent = this.__currencyLabel; + this.__currencyDisplayNode.textContent = this.__currencyLabel; this._calculateValues({ source: null }); } else { - this._currencyDisplayNode = undefined; + this.__currencyDisplayNode = undefined; } this.__setCurrencyDisplayLabel(); } - /** - * @returns the current currency display node - * @protected - */ - get _currencyDisplayNode() { - return Array.from(this.children).find(child => child.slot === 'after'); - } - - /** - * @protected - */ - set _currencyDisplayNode(node) { - if (node) { - this.appendChild(node); - } else { - this._currencyDisplayNode?.remove(); - } - } - /** * @returns a newly created node for displaying the currency * @protected @@ -154,13 +135,32 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { return el; } + /** + * @returns the current currency display node + * @private + */ + get __currencyDisplayNode() { + return Array.from(this.children).find(child => child.slot === 'after'); + } + + /** + * @private + */ + set __currencyDisplayNode(node) { + if (node) { + this.appendChild(node); + } else { + this.__currencyDisplayNode?.remove(); + } + } + /** @private */ __setCurrencyDisplayLabel() { // TODO: (@erikkroes) for optimal a11y, abbreviations should be part of aria-label // example, for a language switch with text 'en', an aria-label of 'english' is not // sufficient, it should also contain the abbreviation. - if (this._currencyDisplayNode) { - this._currencyDisplayNode.setAttribute( + if (this.__currencyDisplayNode) { + this.__currencyDisplayNode.setAttribute( 'aria-label', this.currency ? getCurrencyName(this.currency, {}) : '', ); diff --git a/packages/input-amount/test/lion-input-amount.test.js b/packages/input-amount/test/lion-input-amount.test.js index ebfd3c7b3..27cae4065 100644 --- a/packages/input-amount/test/lion-input-amount.test.js +++ b/packages/input-amount/test/lion-input-amount.test.js @@ -256,19 +256,25 @@ describe('', () => { const el = /** @type {LionInputAmount} */ ( await fixture(``) ); - expect(el._currencyDisplayNode?.getAttribute('data-label')).to.be.not.null; + const label = /** @type {HTMLElement[]} */ (Array.from(el.children)).find( + child => child.slot === 'after', + ); + expect(label?.getAttribute('data-label')).to.be.not.null; const { _inputNode } = getInputMembers(/** @type {* & LionInput} */ (el)); - expect(_inputNode.getAttribute('aria-labelledby')).to.contain(el._currencyDisplayNode?.id); + expect(_inputNode.getAttribute('aria-labelledby')).to.contain(label?.id); }); it('adds an aria-label to currency slot', async () => { const el = /** @type {LionInputAmount} */ ( await fixture(``) ); - expect(el._currencyDisplayNode?.getAttribute('aria-label')).to.equal('euros'); + const label = /** @type {HTMLElement[]} */ (Array.from(el.children)).find( + child => child.slot === 'after', + ); + expect(label?.getAttribute('aria-label')).to.equal('euros'); el.currency = 'USD'; await el.updateComplete; - expect(el._currencyDisplayNode?.getAttribute('aria-label')).to.equal('US dollars'); + expect(label?.getAttribute('aria-label')).to.equal('US dollars'); el.currency = 'PHP'; await el.updateComplete; // TODO: Chrome Intl now thinks this should be pesos instead of pisos. They're probably right. From 627490aedc30c551bdf50266fbf879fd62c2f4cb Mon Sep 17 00:00:00 2001 From: Felix Mueller Date: Tue, 3 Aug 2021 17:04:03 +0200 Subject: [PATCH 3/4] Add test for slot=after element --- .../test/lion-input-amount.test.js | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/input-amount/test/lion-input-amount.test.js b/packages/input-amount/test/lion-input-amount.test.js index 27cae4065..effc607f6 100644 --- a/packages/input-amount/test/lion-input-amount.test.js +++ b/packages/input-amount/test/lion-input-amount.test.js @@ -230,6 +230,28 @@ describe('', () => { expect(currLabel?.getAttribute('aria-label')).to.equal('euros'); }); + it('sets currency label on the after after element', async () => { + const el = /** @type {LionInputAmount} */ ( + await fixture(` + + Currency, please + `) + ); + const mySlotLabel = /** @type {HTMLElement[]} */ (Array.from(el.children)).find( + child => child.slot === 'after', + ); + expect(mySlotLabel?.id).to.equal('123'); + + el.currency = 'EUR'; + await el.updateComplete; + const currLabel = /** @type {HTMLElement[]} */ (Array.from(el.children)).find( + child => child.slot === 'after', + ); + expect(currLabel).to.equal(mySlotLabel); + expect(currLabel?.id).to.equal('123'); + expect(currLabel?.innerText).to.equal('EUR'); + }); + describe('Accessibility', () => { it('is accessible', async () => { const el = await fixture( From 55efd3c64b27244a409bcd2b30129163c35312f8 Mon Sep 17 00:00:00 2001 From: jorenbroekema Date: Tue, 10 Aug 2021 11:08:56 +0200 Subject: [PATCH 4/4] chore: refactor code a little to use slotmixin --- .changeset/nervous-lions-trade.md | 5 ++ packages/input-amount/src/LionInputAmount.js | 88 ++++++++++++------- .../test/lion-input-amount.test.js | 3 +- 3 files changed, 63 insertions(+), 33 deletions(-) create mode 100644 .changeset/nervous-lions-trade.md diff --git a/.changeset/nervous-lions-trade.md b/.changeset/nervous-lions-trade.md new file mode 100644 index 000000000..22cfd6a0a --- /dev/null +++ b/.changeset/nervous-lions-trade.md @@ -0,0 +1,5 @@ +--- +'@lion/input-amount': patch +--- + +Empty or invalid currency now removes the currency label node, this is restored when the currency is valid again. diff --git a/packages/input-amount/src/LionInputAmount.js b/packages/input-amount/src/LionInputAmount.js index a2a62efca..93ebcff85 100644 --- a/packages/input-amount/src/LionInputAmount.js +++ b/packages/input-amount/src/LionInputAmount.js @@ -30,6 +30,20 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { }; } + get slots() { + return { + ...super.slots, + after: () => { + const el = document.createElement('span'); + // The data-label attribute will make sure that FormControl adds this to + // input[aria-labelledby] + el.setAttribute('data-label', ''); + el.textContent = this.__currencyLabel; + return el; + }, + }; + } + static get styles() { return [ ...super.styles, @@ -49,6 +63,7 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { this.currency = undefined; /** @type {string | undefined} */ this.locale = undefined; + this.__currencyDisplayNodeIsConnected = true; this.defaultValidators.push(new IsNumber()); } @@ -80,6 +95,20 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { } } + /** + * Upon connecting slot mixin, we should check if + * the after slot was created by the slot mixin, + * and if so, we should execute the currency changed flow + * which evaluates whether the slot node should be + * removed for invalid currencies + */ + _connectSlotMixin() { + super._connectSlotMixin(); + if (this._isPrivateSlot('after')) { + this._onCurrencyChanged({ currency: this.currency || null }); + } + } + /** * @param {string} newLocale * @param {string} oldLocale @@ -108,31 +137,34 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { * @protected */ _onCurrencyChanged({ currency }) { + if (!this.__currencyDisplayNode) { + return; + } + this.formatOptions.currency = currency || undefined; - if (this.currency) { - if (!this.__currencyDisplayNode) { - this.__currencyDisplayNode = this._createCurrencyDisplayNode(); + if (currency) { + if (!this.__currencyDisplayNodeIsConnected) { + this.appendChild(this.__currencyDisplayNode); + this.__currencyDisplayNodeIsConnected = true; } this.__currencyDisplayNode.textContent = this.__currencyLabel; - this._calculateValues({ source: null }); - } else { - this.__currencyDisplayNode = undefined; - } - this.__setCurrencyDisplayLabel(); - } - /** - * @returns a newly created node for displaying the currency - * @protected - */ - _createCurrencyDisplayNode() { - const el = document.createElement('span'); - // The data-label attribute will make sure that FormControl adds this to - // input[aria-labelledby] - el.setAttribute('data-label', ''); - el.textContent = this.__currencyLabel; - el.slot = 'after'; - return el; + try { + this._calculateValues({ source: null }); + } catch (e) { + // In case Intl.NumberFormat gives error for invalid currency + // we should catch, remove the node, and rethrow (since it's still a user error) + if (e instanceof RangeError) { + this.__currencyDisplayNode?.remove(); + this.__currencyDisplayNodeIsConnected = false; + } + throw e; + } + this.__setCurrencyDisplayLabel(); + } else { + this.__currencyDisplayNode?.remove(); + this.__currencyDisplayNodeIsConnected = false; + } } /** @@ -140,18 +172,12 @@ export class LionInputAmount extends LocalizeMixin(LionInput) { * @private */ get __currencyDisplayNode() { - return Array.from(this.children).find(child => child.slot === 'after'); - } - - /** - * @private - */ - set __currencyDisplayNode(node) { + const node = Array.from(this.children).find(child => child.slot === 'after'); if (node) { - this.appendChild(node); - } else { - this.__currencyDisplayNode?.remove(); + this.__storedCurrencyDisplayNode = node; } + + return node || this.__storedCurrencyDisplayNode; } /** @private */ diff --git a/packages/input-amount/test/lion-input-amount.test.js b/packages/input-amount/test/lion-input-amount.test.js index effc607f6..d3867f510 100644 --- a/packages/input-amount/test/lion-input-amount.test.js +++ b/packages/input-amount/test/lion-input-amount.test.js @@ -134,7 +134,6 @@ describe('', () => { const el = /** @type {LionInputAmount} */ ( await fixture(``) ); - expect( /** @type {HTMLElement[]} */ (Array.from(el.children)).find(child => child.slot === 'after') ?.innerText, @@ -230,7 +229,7 @@ describe('', () => { expect(currLabel?.getAttribute('aria-label')).to.equal('euros'); }); - it('sets currency label on the after after element', async () => { + it('sets currency label on the after element', async () => { const el = /** @type {LionInputAmount} */ ( await fixture(`