From e397f8d68b44c2ccb6447a908a97ace6568738ad Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Wed, 18 Mar 2020 14:17:39 +0100 Subject: [PATCH] feat(field): align (pre)filled and empty, fix filled not working BREAKING: _isPrefilled was removed in favor of _isEmpty. We used to have both, but we decided to align, because they basically do the same thing but opposite. If you were using _isPrefilled, switch to using _isEmpty and just use it in reverse. This change also makes _isEmpty available to all things that implement FormControlMixin. Lastly, filled is available now on all fields that implement InteractionStateMixin --- packages/choice-input/src/ChoiceGroupMixin.js | 5 +- packages/choice-input/src/ChoiceInputMixin.js | 12 +---- packages/field/src/FormControlMixin.js | 22 ++++++++- packages/field/src/InteractionStateMixin.js | 46 +++++++++---------- packages/field/src/LionField.js | 18 ++------ .../InteractionStateMixin.suite.js | 16 +++++-- packages/field/test/lion-field.test.js | 26 +++-------- packages/fieldset/src/FormGroupMixin.js | 8 ++-- packages/fieldset/test/lion-fieldset.test.js | 15 +++--- .../35-System-InteractionStates.stories.mdx | 4 +- packages/validate/src/ValidateMixin.js | 13 ++++-- .../test-suites/ValidateMixin.suite.js | 4 +- 12 files changed, 94 insertions(+), 95 deletions(-) diff --git a/packages/choice-input/src/ChoiceGroupMixin.js b/packages/choice-input/src/ChoiceGroupMixin.js index 85402e9b5..30445f8a0 100644 --- a/packages/choice-input/src/ChoiceGroupMixin.js +++ b/packages/choice-input/src/ChoiceGroupMixin.js @@ -95,15 +95,14 @@ export const ChoiceGroupMixin = dedupeMixin( } _isEmpty() { - const value = this.modelValue; if (this.multipleChoice) { return this.modelValue.length === 0; } - if (typeof value === 'string' && value === '') { + if (typeof this.modelValue === 'string' && this.modelValue === '') { return true; } - if (value === undefined || value === null) { + if (this.modelValue === undefined || this.modelValue === null) { return true; } return false; diff --git a/packages/choice-input/src/ChoiceInputMixin.js b/packages/choice-input/src/ChoiceInputMixin.js index 787c8cd7e..44415fc41 100644 --- a/packages/choice-input/src/ChoiceInputMixin.js +++ b/packages/choice-input/src/ChoiceInputMixin.js @@ -1,6 +1,6 @@ /* eslint-disable class-methods-use-this */ -import { html, css, nothing } from '@lion/core'; +import { css, html, nothing } from '@lion/core'; import { FormatMixin } from '@lion/field'; export const ChoiceInputMixin = superclass => @@ -151,16 +151,6 @@ export const ChoiceInputMixin = superclass => } } - /** - * @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; - } - /** * @override * This method is overridden from FormatMixin. It originally fired the normalizing diff --git a/packages/field/src/FormControlMixin.js b/packages/field/src/FormControlMixin.js index 2bb8d4ddf..045298016 100644 --- a/packages/field/src/FormControlMixin.js +++ b/packages/field/src/FormControlMixin.js @@ -1,4 +1,5 @@ -import { html, css, nothing, dedupeMixin, SlotMixin } from '@lion/core'; +import { css, dedupeMixin, html, nothing, SlotMixin } from '@lion/core'; +import { Unparseable } from '@lion/validate'; import { FormRegisteringMixin } from './registration/FormRegisteringMixin.js'; import { getAriaElementsInRightDomOrder } from './utils/getAriaElementsInRightDomOrder.js'; @@ -392,6 +393,25 @@ export const FormControlMixin = dedupeMixin( `; } + _isEmpty(modelValue = this.modelValue) { + let value = modelValue; + if (this.modelValue instanceof Unparseable) { + value = this.modelValue.viewValue; + } + + // Checks for empty platform types: Objects, Arrays, Dates + if (typeof value === 'object' && value !== null && !(value instanceof Date)) { + return !Object.keys(value).length; + } + + // eslint-disable-next-line no-mixed-operators + // Checks for empty platform types: Numbers, Booleans + const isNumberValue = typeof value === 'number' && (value === 0 || Number.isNaN(value)); + const isBooleanValue = typeof value === 'boolean' && value === false; + + return !value && !isNumberValue && !isBooleanValue; + } + // eslint-disable-next-line class-methods-use-this _feedbackTemplate() { return html` diff --git a/packages/field/src/InteractionStateMixin.js b/packages/field/src/InteractionStateMixin.js index dfb2ad891..b8c38a703 100644 --- a/packages/field/src/InteractionStateMixin.js +++ b/packages/field/src/InteractionStateMixin.js @@ -1,5 +1,5 @@ import { dedupeMixin } from '@lion/core'; -import { Unparseable } from '@lion/validate'; +import { FormControlMixin } from './FormControlMixin.js'; /** * @desc `InteractionStateMixin` adds meta information about touched and dirty states, that can @@ -14,7 +14,7 @@ import { Unparseable } from '@lion/validate'; export const InteractionStateMixin = dedupeMixin( superclass => // eslint-disable-next-line no-unused-vars, no-shadow - class InteractionStateMixin extends superclass { + class InteractionStateMixin extends FormControlMixin(superclass) { static get properties() { return { /** @@ -31,6 +31,13 @@ export const InteractionStateMixin = dedupeMixin( type: Boolean, reflect: true, }, + /** + * True when the modelValue is non-empty (see _isEmpty in FormControlMixin) + */ + filled: { + type: Boolean, + reflect: true, + }, /** * True when user has left non-empty field or input is prefilled. * The name must be seen from the point of view of the input field: @@ -55,36 +62,25 @@ export const InteractionStateMixin = dedupeMixin( this._onTouchedChanged(); } + if (name === 'modelValue') { + // We do this in _requestUpdate because we don't want to fire another re-render (e.g. when doing this in updated) + // Furthermore, we cannot do it on model-value-changed event because it isn't fired initially. + this.filled = !this._isEmpty(); + } + if (name === 'dirty' && this.dirty !== oldVal) { this._onDirtyChanged(); } } - static _isPrefilled(modelValue) { - let value = modelValue; - if (modelValue instanceof Unparseable) { - value = modelValue.viewValue; - } - // Checks for empty platform types: Objects, Arrays, Dates - if (typeof value === 'object' && value !== null && !(value instanceof Date)) { - return !!Object.keys(value).length; - } - // eslint-disable-next-line no-mixed-operators - // Checks for empty platform types: Numbers, Booleans - const isNumberValue = typeof value === 'number' && (value === 0 || Number.isNaN(value)); - const isBooleanValue = typeof value === 'boolean' && value === false; - - return !!value || isNumberValue || isBooleanValue; - } - constructor() { super(); this.touched = false; this.dirty = false; this.prefilled = false; + this.filled = false; this._leaveEvent = 'blur'; this._valueChangedEvent = 'model-value-changed'; - this._iStateOnLeave = this._iStateOnLeave.bind(this); this._iStateOnValueChange = this._iStateOnValueChange.bind(this); } @@ -118,23 +114,23 @@ export const InteractionStateMixin = dedupeMixin( */ initInteractionState() { this.dirty = false; - this.prefilled = this.constructor._isPrefilled(this.modelValue); + this.prefilled = !this._isEmpty(); } /** * Sets touched value to true * Reevaluates prefilled state. * When false, on next interaction, user will start with a clean state. - * @private + * @protected */ _iStateOnLeave() { this.touched = true; - this.prefilled = this.constructor._isPrefilled(this.modelValue); + this.prefilled = !this._isEmpty(); } /** * Sets dirty value and validates when already touched or invalid - * @private + * @protected */ _iStateOnValueChange() { this.dirty = true; @@ -146,7 +142,7 @@ export const InteractionStateMixin = dedupeMixin( resetInteractionState() { this.touched = false; this.dirty = false; - this.prefilled = this.constructor._isPrefilled(this.modelValue); + this.prefilled = !this._isEmpty(); } _onTouchedChanged() { diff --git a/packages/field/src/LionField.js b/packages/field/src/LionField.js index 868d0e0a6..70796c3cf 100644 --- a/packages/field/src/LionField.js +++ b/packages/field/src/LionField.js @@ -1,10 +1,10 @@ -import { SlotMixin, LitElement } from '@lion/core'; +import { LitElement, SlotMixin } from '@lion/core'; import { DisabledMixin } from '@lion/core/src/DisabledMixin.js'; import { ValidateMixin } from '@lion/validate'; +import { FocusMixin } from './FocusMixin.js'; +import { FormatMixin } from './FormatMixin.js'; import { FormControlMixin } from './FormControlMixin.js'; import { InteractionStateMixin } from './InteractionStateMixin.js'; // applies FocusMixin -import { FormatMixin } from './FormatMixin.js'; -import { FocusMixin } from './FocusMixin.js'; /* eslint-disable wc/guard-super-call */ @@ -80,7 +80,6 @@ export class LionField extends FormControlMixin( if (this._inputNode) { this._setValueAndPreserveCaret(value); } - this._onValueChanged({ value }); } get value() { @@ -107,7 +106,6 @@ export class LionField extends FormControlMixin( // FormatMixin this._delegateInitialValueAttr(); super.connectedCallback(); - this._onChange = this._onChange.bind(this); this._inputNode.addEventListener('change', this._onChange); this.classList.add('form-field'); // eslint-disable-line @@ -178,16 +176,6 @@ export class LionField extends FormControlMixin( ); } - _onValueChanged({ value }) { - if (super._onValueChanged) { - super._onValueChanged(); - } - // For styling purposes, make it known the input field is not empty - if (this._inputNode) { - this[value ? 'setAttribute' : 'removeAttribute']('filled', ''); - } - } - /** * Restores the cursor to its original position after updating the value. * @param {string} newValue The value that should be saved. diff --git a/packages/field/test-suites/InteractionStateMixin.suite.js b/packages/field/test-suites/InteractionStateMixin.suite.js index fc0ccd5e7..7a38202a3 100644 --- a/packages/field/test-suites/InteractionStateMixin.suite.js +++ b/packages/field/test-suites/InteractionStateMixin.suite.js @@ -31,9 +31,8 @@ export function runInteractionStateMixinSuite(customConfig) { set modelValue(v) { this._modelValue = v; - this.dispatchEvent( - new CustomEvent('model-value-changed', { bubbles: true, composed: true }), - ); + this.dispatchEvent(new CustomEvent('model-value-changed', { bubbles: true })); + this.requestUpdate('modelValue'); } get modelValue() { @@ -80,6 +79,17 @@ export function runInteractionStateMixinSuite(customConfig) { expect(el.hasAttribute('dirty')).to.be.true; }); + it('sets an attribute "filled" if the input has a non-empty modelValue', async () => { + const el = await fixture(html`<${tag} .modelValue=${'hello'}>`); + expect(el.hasAttribute('filled')).to.equal(true); + el.modelValue = ''; + await el.updateComplete; + expect(el.hasAttribute('filled')).to.equal(false); + el.modelValue = 'foo'; + await el.updateComplete; + expect(el.hasAttribute('filled')).to.equal(true); + }); + it('fires "(touched|dirty)-state-changed" event when state changes', async () => { const touchedSpy = sinon.spy(); const dirtySpy = sinon.spy(); diff --git a/packages/field/test/lion-field.test.js b/packages/field/test/lion-field.test.js index b45fe156f..4aba25277 100644 --- a/packages/field/test/lion-field.test.js +++ b/packages/field/test/lion-field.test.js @@ -1,18 +1,17 @@ +import { unsafeHTML } from '@lion/core'; +import { localize } from '@lion/localize'; +import { localizeTearDown } from '@lion/localize/test-helpers.js'; +import { Required, Validator } from '@lion/validate'; import { + aTimeout, expect, fixture, html, - unsafeStatic, - triggerFocusFor, triggerBlurFor, - aTimeout, + triggerFocusFor, + unsafeStatic, } from '@open-wc/testing'; -import { unsafeHTML } from '@lion/core'; import sinon from 'sinon'; -import { Validator, Required } from '@lion/validate'; -import { localize } from '@lion/localize'; -import { localizeTearDown } from '@lion/localize/test-helpers.js'; - import '../lion-field.js'; const tagString = 'lion-field'; @@ -153,17 +152,6 @@ describe('', () => { expect(el._inputNode.getAttribute('autocomplete')).to.equal('off'); }); - it('has an attribute filled if this.value is filled', async () => { - const el = await fixture(html`<${tag} value="filled">${inputSlot}`); - expect(el.hasAttribute('filled')).to.equal(true); - el.value = ''; - await el.updateComplete; - expect(el.hasAttribute('filled')).to.equal(false); - el.value = 'bla'; - await el.updateComplete; - expect(el.hasAttribute('filled')).to.equal(true); - }); - it('preserves the caret position on value change for native text fields (input|textarea)', async () => { const el = await fixture(html`<${tag}>${inputSlot}`); await triggerFocusFor(el); diff --git a/packages/fieldset/src/FormGroupMixin.js b/packages/fieldset/src/FormGroupMixin.js index d328cfa35..4fc46997a 100644 --- a/packages/fieldset/src/FormGroupMixin.js +++ b/packages/fieldset/src/FormGroupMixin.js @@ -1,6 +1,6 @@ -import { html, dedupeMixin, SlotMixin } from '@lion/core'; +import { dedupeMixin, html, SlotMixin } from '@lion/core'; import { DisabledMixin } from '@lion/core/src/DisabledMixin.js'; -import { FormControlMixin, FormRegistrarMixin, FormControlsCollection } from '@lion/field'; +import { FormControlMixin, FormControlsCollection, FormRegistrarMixin } from '@lion/field'; import { getAriaElementsInRightDomOrder } from '@lion/field/src/utils/getAriaElementsInRightDomOrder.js'; import { ValidateMixin } from '@lion/validate'; import { FormElementsHaveNoError } from './FormElementsHaveNoError.js'; @@ -361,7 +361,7 @@ export const FormGroupMixin = dedupeMixin( } // TODO: Unlink in removeFormElement this.__linkChildrenMessagesToParent(child); - this.validate(); + this.validate({ clearCurrentResult: true }); } /** @@ -392,7 +392,7 @@ export const FormGroupMixin = dedupeMixin( */ removeFormElement(...args) { super.removeFormElement(...args); - this.validate(); + this.validate({ clearCurrentResult: true }); } }, ); diff --git a/packages/fieldset/test/lion-fieldset.test.js b/packages/fieldset/test/lion-fieldset.test.js index e0b05f6fb..53e7d1562 100644 --- a/packages/fieldset/test/lion-fieldset.test.js +++ b/packages/fieldset/test/lion-fieldset.test.js @@ -1,19 +1,19 @@ +import { LionField } from '@lion/field'; +import '@lion/field/lion-field.js'; +import { localizeTearDown } from '@lion/localize/test-helpers.js'; +import { IsNumber, Validator } from '@lion/validate'; import { + defineCE, expect, fixtureSync, html, - unsafeStatic, - triggerFocusFor, nextFrame, - defineCE, + triggerFocusFor, + unsafeStatic, } from '@open-wc/testing'; import { formFixture as fixture } from '@lion/field/test-helpers.js'; import sinon from 'sinon'; -import { Validator, IsNumber } from '@lion/validate'; -import { localizeTearDown } from '@lion/localize/test-helpers.js'; import '../lion-fieldset.js'; -import { LionField } from '@lion/field'; -import '@lion/field/lion-field.js'; const childTagString = defineCE( class extends LionField { @@ -434,6 +434,7 @@ describe('', () => { // Edge case: remove all children el.removeChild(el.querySelector('[id=c1]')); await nextFrame(); + expect(el.validationStates.error.HasEvenNumberOfChildren).to.equal(undefined); }); }); diff --git a/packages/form-system/stories/35-System-InteractionStates.stories.mdx b/packages/form-system/stories/35-System-InteractionStates.stories.mdx index 9ceef3c92..36d7e5391 100644 --- a/packages/form-system/stories/35-System-InteractionStates.stories.mdx +++ b/packages/form-system/stories/35-System-InteractionStates.stories.mdx @@ -60,8 +60,8 @@ The example below shows how this is done for checkboxes/radio-inputs. /** * @override */ -static _isPrefilled(modelValue) { - return modelValue.checked; +_isEmpty() { + return !this.checked; } ``` diff --git a/packages/validate/src/ValidateMixin.js b/packages/validate/src/ValidateMixin.js index 08144d29b..6185e0718 100644 --- a/packages/validate/src/ValidateMixin.js +++ b/packages/validate/src/ValidateMixin.js @@ -1,6 +1,6 @@ /* eslint-disable class-methods-use-this, camelcase, no-param-reassign, max-classes-per-file */ -import { dedupeMixin, SlotMixin, ScopedElementsMixin } from '@lion/core'; +import { dedupeMixin, ScopedElementsMixin, SlotMixin } from '@lion/core'; import { localize } from '@lion/localize'; import { LionValidationFeedback } from './LionValidationFeedback.js'; import { ResultValidator } from './ResultValidator.js'; @@ -307,10 +307,12 @@ export const ValidateMixin = dedupeMixin( * - the validatity is dependent on the formControl type and therefore determined * by the formControl.__isEmpty method. Basically, the Required Validator is a means * to trigger formControl.__isEmpty. - * - when __isEmpty returns false, the input was empty. This means we need to stop + * - when __isEmpty returns true, the input was empty. This means we need to stop * validation here, because all other Validators' execute functions assume the * value is not empty (there would be nothing to validate). */ + // TODO: Try to remove this when we have a single lion form core package, because then we can + // depend on FormControlMixin directly, and _isEmpty will always be an existing method on the prototype then const isEmpty = this.__isEmpty(value); if (isEmpty) { if (requiredValidator) { @@ -423,6 +425,7 @@ export const ValidateMixin = dedupeMixin( validationStates[v.type][v.constructor.name] = true; }); this.validationStates = validationStates; + this.hasFeedbackFor = [...new Set(this.__validationResult.map(v => v.type))]; /** private event that should be listened to by LionFieldSet */ @@ -485,7 +488,11 @@ export const ValidateMixin = dedupeMixin( // if (typeof this.__isRequired === 'function') { // return !this.__isRequired(v); // } - return v === null || typeof v === 'undefined' || v === ''; + return ( + this.modelValue === null || + typeof this.modelValue === 'undefined' || + this.modelValue === '' + ); } // ------------------------------------------------------------------------------------------ diff --git a/packages/validate/test-suites/ValidateMixin.suite.js b/packages/validate/test-suites/ValidateMixin.suite.js index 3ee9baa92..d66c29ef6 100644 --- a/packages/validate/test-suites/ValidateMixin.suite.js +++ b/packages/validate/test-suites/ValidateMixin.suite.js @@ -665,8 +665,8 @@ export function runValidateMixinSuite(customConfig) { it('calls "._isEmpty" when provided (useful for different modelValues)', async () => { const customRequiredTagString = defineCE( class extends ValidateMixin(LitElement) { - _isEmpty(modelValue) { - return modelValue.model === ''; + _isEmpty() { + return this.modelValue.model === ''; } }, );