From 2870e0d95c31ac7cce6f7d3a4e97a4207bb7fd8b Mon Sep 17 00:00:00 2001 From: Thijs Louisse Date: Mon, 24 Jun 2019 16:49:45 +0200 Subject: [PATCH] feat(choice-input): api normalization and cleanup --- packages/choice-input/package.json | 3 +- packages/choice-input/src/ChoiceInputMixin.js | 192 +++++++++++------- .../test/ChoiceInputMixin.test.js | 143 ++++++++++--- 3 files changed, 243 insertions(+), 95 deletions(-) diff --git a/packages/choice-input/package.json b/packages/choice-input/package.json index 8cd109cfc..03038ea50 100644 --- a/packages/choice-input/package.json +++ b/packages/choice-input/package.json @@ -38,6 +38,7 @@ "devDependencies": { "@lion/input": "^0.1.22", "@open-wc/demoing-storybook": "^0.2.0", - "@open-wc/testing": "^0.12.5" + "@open-wc/testing": "^0.12.5", + "sinon": "^7.2.2" } } diff --git a/packages/choice-input/src/ChoiceInputMixin.js b/packages/choice-input/src/ChoiceInputMixin.js index e85dfa002..37a0346a4 100644 --- a/packages/choice-input/src/ChoiceInputMixin.js +++ b/packages/choice-input/src/ChoiceInputMixin.js @@ -1,70 +1,94 @@ /* eslint-disable class-methods-use-this */ import { html, css, nothing } from '@lion/core'; -import { ObserverMixin } from '@lion/core/src/ObserverMixin.js'; import { FormatMixin } from '@lion/field'; export const ChoiceInputMixin = superclass => // eslint-disable-next-line - class ChoiceInputMixin extends FormatMixin(ObserverMixin(superclass)) { - get delegations() { + class ChoiceInputMixin extends FormatMixin(superclass) { + static get properties() { return { - ...super.delegations, - target: () => this.inputElement, - properties: [...super.delegations.properties, 'checked'], - attributes: [...super.delegations.attributes, 'checked'], + ...super.properties, + /** + * Boolean indicating whether or not this element is checked by the end user. + */ + checked: { + type: Boolean, + reflect: true, + }, + /** + * Whereas 'normal' `.modelValue`s usually store a complex/typed version + * of a view value, choice inputs have a slightly different approach. + * In order to remain their Single Source of Truth characteristic, choice inputs + * store both the value and 'checkedness', in the format { value: 'x', checked: true } + * Different from the platform, this also allows to serialize the 'non checkedness', + * allowing to restore form state easily and inform the server about unchecked options. + */ + modelValue: { + type: Object, + hasChanged: (nw, old = {}) => nw.value !== old.value || nw.checked !== old.checked, + }, + /** + * The value property of the modelValue. It provides an easy inteface for storing + * (complex) values in the modelValue + */ + choiceValue: { + type: Object, + }, }; } - static get syncObservers() { - return { - ...super.syncObservers, - _syncModelValueToChecked: ['modelValue'], - }; - } - - static get asyncObservers() { - return { - ...super.asyncObservers, - _reflectCheckedToCssClass: ['modelValue'], - }; - } - - get choiceChecked() { - return this.modelValue.checked; - } - - set choiceChecked(checked) { - if (this.modelValue.checked !== checked) { - this.modelValue = { value: this.modelValue.value, checked }; - } - } - get choiceValue() { return this.modelValue.value; } set choiceValue(value) { + this.requestUpdate('choiceValue', this.choiceValue); if (this.modelValue.value !== value) { this.modelValue = { value, checked: this.modelValue.checked }; } } + _requestUpdate(name, oldValue) { + super._requestUpdate(name, oldValue); + + if (name === 'modelValue') { + if (this.modelValue.checked !== this.checked) { + this.__syncModelCheckedToChecked(this.modelValue.checked); + } + } else if (name === 'checked') { + if (this.modelValue.checked !== this.checked) { + this.__syncCheckedToModel(this.checked); + } + } + } + + firstUpdated(c) { + super.firstUpdated(c); + if (c.has('checked')) { + // Here we set the initial value for our [slot=input] content, + // which has been set by our SlotMixin + this.__syncCheckedToInputElement(); + } + } + + updated(c) { + super.updated(c); + if (c.has('modelValue')) { + this._reflectCheckedToCssClass({ modelValue: this.modelValue }); + this.__syncCheckedToInputElement(); + } + } + constructor() { super(); this.modelValue = { value: '', checked: false }; } /** - * @override - * Override InteractionStateMixin - * 'prefilled' should be false when modelValue is { checked: false }, which would return - * true in original method (since non-empty objects are considered prefilled by default). + * Styles for [input=radio] and [input=checkbox] wrappers. + * For [role=option] extensions, please override completely */ - static _isPrefilled(modelValue) { - return modelValue.checked; - } - static get styles() { return [ css` @@ -79,6 +103,10 @@ export const ChoiceInputMixin = superclass => ]; } + /** + * Template for [input=radio] and [input=checkbox] wrappers. + * For [role=option] extensions, please override completely + */ render() { return html` @@ -96,22 +124,44 @@ export const ChoiceInputMixin = superclass => } connectedCallback() { - if (super.connectedCallback) super.connectedCallback(); - this.addEventListener('user-input-changed', this._toggleChecked); + super.connectedCallback(); + this.addEventListener('user-input-changed', this.__toggleChecked); this._reflectCheckedToCssClass(); } disconnectedCallback() { - if (super.disconnectedCallback) super.disconnectedCallback(); - this.removeEventListener('user-input-changed', this._toggleChecked); + super.disconnectedCallback(); + this.removeEventListener('user-input-changed', this.__toggleChecked); } - _toggleChecked() { - this.choiceChecked = !this.choiceChecked; + __toggleChecked() { + this.checked = !this.checked; } - _syncModelValueToChecked({ modelValue }) { - this.checked = !!modelValue.checked; + __syncModelCheckedToChecked(checked) { + this.checked = checked; + } + + __syncCheckedToModel(checked) { + this.modelValue = { value: this.choiceValue, checked }; + } + + __syncCheckedToInputElement() { + // .inputElement might not be available yet(slot content) + // or at all (no reliance on platform construct, in case of [role=option]) + if (this.inputElement) { + this.inputElement.checked = this.checked; + } + } + + /** + * @override + * Override InteractionStateMixin + * 'prefilled' should be false when modelValue is { checked: false }, which would return + * true in original method (since non-empty objects are considered prefilled by default). + */ + static _isPrefilled(modelValue) { + return modelValue.checked; } /** @@ -126,30 +176,13 @@ export const ChoiceInputMixin = superclass => /** * @override - * Override FormatMixin default dispatching of model-value-changed as it only does a simple - * comparision which is not enough in js because - * { value: 'foo', checked: true } !== { value: 'foo', checked: true } - * We do our own "deep" comparision. - * - * @param {object} modelValue - * @param {object} modelValue the old one + * hasChanged is designed for async (updated) callback, also check for sync + * (_requestUpdate) callback */ - // TODO: consider making a generic option inside FormatMixin for deep object comparisons when - // modelValue is an object - _dispatchModelValueChangedEvent({ modelValue }, { modelValue: old }) { - let changed = true; - if (old) { - changed = modelValue.value !== old.value || modelValue.checked !== old.checked; + _onModelValueChanged({ modelValue }, { modelValue: old }) { + if (this.constructor._classProperties.get('modelValue').hasChanged(modelValue, old)) { + super._onModelValueChanged({ modelValue }); } - if (changed) { - this.dispatchEvent( - new CustomEvent('model-value-changed', { bubbles: true, composed: true }), - ); - } - } - - _reflectCheckedToCssClass() { - this.classList[this.choiceChecked ? 'add' : 'remove']('state-checked'); } /** @@ -171,11 +204,30 @@ export const ChoiceInputMixin = superclass => /** * @override - * Overridden from ValidateMixin, since a different modelValue is used for choice inputs. + * Overridden from Field, since a different modelValue is used for choice inputs. */ __isRequired(modelValue) { return { required: !!modelValue.checked, }; } + + /** + * @deprecated use .checked + */ + get choiceChecked() { + return this.checked; + } + + /** + * @deprecated use .checked + */ + set choiceChecked(c) { + this.checked = c; + } + + /** @deprecated for styling purposes, use [checked] attribute */ + _reflectCheckedToCssClass() { + this.classList[this.checked ? 'add' : 'remove']('state-checked'); + } }; diff --git a/packages/choice-input/test/ChoiceInputMixin.test.js b/packages/choice-input/test/ChoiceInputMixin.test.js index 34f5ede3c..3d41e2caf 100644 --- a/packages/choice-input/test/ChoiceInputMixin.test.js +++ b/packages/choice-input/test/ChoiceInputMixin.test.js @@ -1,5 +1,6 @@ import { expect, fixture } from '@open-wc/testing'; import { html } from '@lion/core'; +import sinon from 'sinon'; import { LionInput } from '@lion/input'; import { ChoiceInputMixin } from '../src/ChoiceInputMixin.js'; @@ -50,11 +51,11 @@ describe('ChoiceInputMixin', () => { `); expect(counter).to.equal(1); // undefined to set value - el.choiceChecked = true; + el.checked = true; expect(counter).to.equal(2); // no change means no event - el.choiceChecked = true; + el.checked = true; el.choiceValue = 'foo'; el.modelValue = { value: 'foo', checked: true }; expect(counter).to.equal(2); @@ -87,58 +88,136 @@ describe('ChoiceInputMixin', () => { `); expect(el.error.required).to.be.true; - el.choiceChecked = true; + el.checked = true; expect(el.error.required).to.be.undefined; }); describe('Checked state synchronization', () => { it('synchronizes checked state initially (via attribute or property)', async () => { const el = await fixture(``); - expect(el.choiceChecked).to.equal(false, 'initially unchecked'); + expect(el.checked).to.equal(false, 'initially unchecked'); const precheckedElementAttr = await fixture(html` - + `); - expect(precheckedElementAttr.choiceChecked).to.equal(true, 'initially checked via attribute'); + el._syncValueUpwards = () => {}; // We need to disable the method for the test to pass + + expect(precheckedElementAttr.checked).to.equal(true, 'initially checked via attribute'); }); it('can be checked and unchecked programmatically', async () => { const el = await fixture(``); - expect(el.choiceChecked).to.be.false; - el.choiceChecked = true; - expect(el.choiceChecked).to.be.true; + expect(el.checked).to.be.false; + el.checked = true; + expect(el.checked).to.be.true; + + await el.updateComplete; + expect(el.inputElement.checked).to.be.true; }); it('can be checked and unchecked via user interaction', async () => { const el = await fixture(``); el.inputElement.click(); - expect(el.choiceChecked).to.be.true; + expect(el.checked).to.be.true; el.inputElement.click(); - expect(el.choiceChecked).to.be.false; + expect(el.checked).to.be.false; }); it('synchronizes modelValue to checked state and vice versa', async () => { const el = await fixture(html` `); - expect(el.choiceChecked).to.be.false; + expect(el.checked).to.be.false; expect(el.modelValue).to.deep.equal({ checked: false, value: 'foo', }); - el.choiceChecked = true; - expect(el.choiceChecked).to.be.true; + el.checked = true; + expect(el.checked).to.be.true; expect(el.modelValue).to.deep.equal({ checked: true, value: 'foo', }); }); - it('synchronizes checked state to class "state-checked" for styling purposes', async () => { + it('ensures optimal synchronize performance by preventing redundant computation steps', async () => { + /* we are checking private apis here to make sure we do not have cyclical updates + which can be quite common for these type of connected data */ + const el = await fixture(html` + + `); + expect(el.checked).to.be.false; + + const spyModelCheckedToChecked = sinon.spy(el, '__syncModelCheckedToChecked'); + const spyCheckedToModel = sinon.spy(el, '__syncCheckedToModel'); + el.checked = true; + expect(el.modelValue.checked).to.be.true; + expect(spyModelCheckedToChecked.callCount).to.equal(0); + expect(spyCheckedToModel.callCount).to.equal(1); + + el.modelValue = { value: 'foo', checked: false }; + expect(el.checked).to.be.false; + expect(spyModelCheckedToChecked.callCount).to.equal(1); + expect(spyCheckedToModel.callCount).to.equal(1); + + // not changeing values should not trigger any updates + el.checked = false; + el.modelValue = { value: 'foo', checked: false }; + expect(spyModelCheckedToChecked.callCount).to.equal(1); + expect(spyCheckedToModel.callCount).to.equal(1); + }); + + it('synchronizes checked state to [checked] attribute for styling purposes', async () => { + const hasAttr = el => el.hasAttribute('checked'); + const el = await fixture(``); + const elChecked = await fixture(html` + + + + `); + + // Initial values + expect(hasAttr(el)).to.equal(false, 'inital unchecked element'); + expect(hasAttr(elChecked)).to.equal(true, 'inital checked element'); + + // Programmatically via checked + el.checked = true; + elChecked.checked = false; + + await el.updateComplete; + expect(hasAttr(el)).to.equal(true, 'programmatically checked'); + expect(hasAttr(elChecked)).to.equal(false, 'programmatically unchecked'); + + // reset + el.checked = false; + elChecked.checked = true; + + // Via user interaction + el.inputElement.click(); + elChecked.inputElement.click(); + await el.updateComplete; + expect(hasAttr(el)).to.equal(true, 'user click checked'); + expect(hasAttr(elChecked)).to.equal(false, 'user click unchecked'); + + // reset + el.checked = false; + elChecked.checked = true; + + // Programmatically via modelValue + el.modelValue = { value: '', checked: true }; + elChecked.modelValue = { value: '', checked: false }; + await el.updateComplete; + expect(hasAttr(el)).to.equal(true, 'modelValue checked'); + expect(hasAttr(elChecked)).to.equal(false, 'modelValue unchecked'); + }); + + it('[deprecated] synchronizes checked state to class "state-checked" for styling purposes', async () => { const hasClass = el => [].slice.call(el.classList).indexOf('state-checked') > -1; const el = await fixture(``); const elChecked = await fixture(html` - + + + `); // Initial values @@ -146,15 +225,16 @@ describe('ChoiceInputMixin', () => { expect(hasClass(elChecked)).to.equal(true, 'inital checked element'); // Programmatically via checked - el.choiceChecked = true; - elChecked.choiceChecked = false; + el.checked = true; + elChecked.checked = false; + await el.updateComplete; expect(hasClass(el)).to.equal(true, 'programmatically checked'); expect(hasClass(elChecked)).to.equal(false, 'programmatically unchecked'); // reset - el.choiceChecked = false; - elChecked.choiceChecked = true; + el.checked = false; + elChecked.checked = true; // Via user interaction el.inputElement.click(); @@ -164,8 +244,8 @@ describe('ChoiceInputMixin', () => { expect(hasClass(elChecked)).to.equal(false, 'user click unchecked'); // reset - el.choiceChecked = false; - elChecked.choiceChecked = true; + el.checked = false; + elChecked.checked = true; // Programmatically via modelValue el.modelValue = { value: '', checked: true }; @@ -174,6 +254,21 @@ describe('ChoiceInputMixin', () => { expect(hasClass(el)).to.equal(true, 'modelValue checked'); expect(hasClass(elChecked)).to.equal(false, 'modelValue unchecked'); }); + + it('[deprecated] uses choiceChecked to set checked state', async () => { + const el = await fixture(html` + + `); + expect(el.choiceChecked).to.be.false; + el.choiceChecked = true; + expect(el.checked).to.be.true; + expect(el.modelValue).to.deep.equal({ + checked: true, + value: 'foo', + }); + await el.updateComplete; + expect(el.inputElement.checked).to.be.true; + }); }); describe('Format/parse/serialize loop', () => { @@ -184,7 +279,7 @@ describe('ChoiceInputMixin', () => { expect(el.modelValue).deep.equal({ value: 'foo', checked: false }); const elChecked = await fixture(html` - + `); expect(elChecked.modelValue).deep.equal({ value: 'foo', checked: true }); }); @@ -203,7 +298,7 @@ describe('ChoiceInputMixin', () => { describe('Interaction states', () => { it('is considered prefilled when checked and not considered prefilled when unchecked', async () => { const el = await fixture(html` - + `); expect(el.prefilled).equal(true, 'checked element not considered prefilled');