From af90b20ed07525ab5c160c29af25e2b7e826e254 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Wed, 10 Mar 2021 17:17:06 +0100 Subject: [PATCH 1/2] fix(form-core): only show success after recovery from shown feedback --- .changeset/orange-walls-sniff.md | 5 + .../form-core/src/validate/ResultValidator.js | 4 +- .../form-core/src/validate/ValidateMixin.js | 24 ++-- .../resultValidators/DefaultSuccess.js | 11 +- .../test-suites/ValidateMixin.suite.js | 20 +-- .../test/validate/ResultValidator.test.js | 9 +- .../types/validate/ValidateMixinTypes.d.ts | 2 +- .../test/form-validation-integrations.test.js | 114 ++++++++++++++++++ 8 files changed, 161 insertions(+), 28 deletions(-) create mode 100644 .changeset/orange-walls-sniff.md create mode 100644 packages/form-integrations/test/form-validation-integrations.test.js diff --git a/.changeset/orange-walls-sniff.md b/.changeset/orange-walls-sniff.md new file mode 100644 index 000000000..394920ee9 --- /dev/null +++ b/.changeset/orange-walls-sniff.md @@ -0,0 +1,5 @@ +--- +'@lion/form-core': patch +--- + +Only show success feedback if the user is recovering from a shown error/warning. diff --git a/packages/form-core/src/validate/ResultValidator.js b/packages/form-core/src/validate/ResultValidator.js index 014eaf290..5b4362339 100644 --- a/packages/form-core/src/validate/ResultValidator.js +++ b/packages/form-core/src/validate/ResultValidator.js @@ -10,12 +10,12 @@ export class ResultValidator extends Validator { /** * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {Validator[]} context.prevValidationResult + * @param {string} context.prevShownValidationFeedback * @param {Validator[]} [context.validators] * @returns {boolean} */ // eslint-disable-next-line no-unused-vars, class-methods-use-this - executeOnResults({ regularValidationResult, prevValidationResult, validators }) { + executeOnResults({ regularValidationResult, prevShownValidationFeedback, validators }) { return true; } } diff --git a/packages/form-core/src/validate/ValidateMixin.js b/packages/form-core/src/validate/ValidateMixin.js index 9fa844b61..66ba3f5d4 100644 --- a/packages/form-core/src/validate/ValidateMixin.js +++ b/packages/form-core/src/validate/ValidateMixin.js @@ -161,8 +161,8 @@ export const ValidateMixinImplementation = superclass => * @type {Validator[]} */ this.__validationResult = []; - /** @type {Validator[]} */ - this.__prevValidationResult = []; + /** @type {string} */ + this.__prevShownValidationFeedback = ''; this.__onValidatorUpdated = this.__onValidatorUpdated.bind(this); this._updateFeedbackComponent = this._updateFeedbackComponent.bind(this); @@ -279,7 +279,6 @@ export const ValidateMixinImplementation = superclass => return; } - this.__storePrevResult(); if (clearCurrentResult) { // Clear ('invalidate') all pending and existing validation results. // This is needed because we have async (pending) validators whose results @@ -289,10 +288,6 @@ export const ValidateMixinImplementation = superclass => await this.__executeValidators(); } - __storePrevResult() { - this.__prevValidationResult = this.__validationResult; - } - /** * @desc step A1-3 + B (as explained in 'validate') */ @@ -401,7 +396,7 @@ export const ValidateMixinImplementation = superclass => return resultValidators.filter(v => v.executeOnResults({ regularValidationResult, - prevValidationResult: this.__prevValidationResult, + prevShownValidationFeedback: this.__prevShownValidationFeedback, }), ); } @@ -582,8 +577,12 @@ export const ValidateMixinImplementation = superclass => this.__prioritizedResult = this._prioritizeAndFilterFeedback({ validationResult: this.__validationResult, }); - const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult); + if (this.__prioritizedResult.length > 0) { + this.__prevShownValidationFeedback = this.__prioritizedResult[0].type; + } + + const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult); _feedbackNode.feedbackData = messageMap.length ? messageMap : []; }); } else { @@ -636,10 +635,15 @@ export const ValidateMixinImplementation = superclass => _updateShouldShowFeedbackFor() { const ctor = /** @type {typeof import('../../types/validate/ValidateMixinTypes').ValidateHost} */ (this .constructor); + // Necessary typecast because types aren't smart enough to understand that we filter out undefined - this.shouldShowFeedbackFor = /** @type {string[]} */ (ctor.validationTypes + const newShouldShowFeedbackFor = /** @type {string[]} */ (ctor.validationTypes .map(type => (this._showFeedbackConditionFor(type) ? type : undefined)) .filter(_ => !!_)); + + if (JSON.stringify(this.shouldShowFeedbackFor) !== JSON.stringify(newShouldShowFeedbackFor)) { + this.shouldShowFeedbackFor = newShouldShowFeedbackFor; + } } /** diff --git a/packages/form-core/src/validate/resultValidators/DefaultSuccess.js b/packages/form-core/src/validate/resultValidators/DefaultSuccess.js index 3673d9939..ad3a9c7dc 100644 --- a/packages/form-core/src/validate/resultValidators/DefaultSuccess.js +++ b/packages/form-core/src/validate/resultValidators/DefaultSuccess.js @@ -17,15 +17,18 @@ export class DefaultSuccess extends ResultValidator { * * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {Validator[]} context.prevValidationResult + * @param {string} context.prevShownValidationFeedback * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this - executeOnResults({ regularValidationResult, prevValidationResult }) { + executeOnResults({ regularValidationResult, prevShownValidationFeedback }) { const errorOrWarning = /** @param {Validator} v */ v => v.type === 'error' || v.type === 'warning'; const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length; - const prevHadErrorOrWarning = !!prevValidationResult.filter(errorOrWarning).length; - return !hasErrorOrWarning && prevHadErrorOrWarning; + + return ( + !hasErrorOrWarning && + (prevShownValidationFeedback === 'error' || prevShownValidationFeedback === 'warning') + ); } } diff --git a/packages/form-core/test-suites/ValidateMixin.suite.js b/packages/form-core/test-suites/ValidateMixin.suite.js index b6973ea08..d27795931 100644 --- a/packages/form-core/test-suites/ValidateMixin.suite.js +++ b/packages/form-core/test-suites/ValidateMixin.suite.js @@ -548,15 +548,21 @@ export function runValidateMixinSuite(customConfig) { /** * - * @param {{ regularValidationResult: Validator[], prevValidationResult: Validator[]}} param0 + * @param {Object} context + * @param {Validator[]} context.regularValidationResult + * @param {string} context.prevShownValidationFeedback + * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this - executeOnResults({ regularValidationResult, prevValidationResult }) { + executeOnResults({ regularValidationResult, prevShownValidationFeedback }) { const errorOrWarning = /** @param {Validator} v */ v => v.type === 'error' || v.type === 'warning'; const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length; - const prevHadErrorOrWarning = !!prevValidationResult.filter(errorOrWarning).length; - return !hasErrorOrWarning && prevHadErrorOrWarning; + + return ( + !hasErrorOrWarning && + (prevShownValidationFeedback === 'error' || prevShownValidationFeedback === 'warning') + ); } }; @@ -607,15 +613,15 @@ export function runValidateMixinSuite(customConfig) { .modelValue=${'myValue'} >${lightDom} `)); - const prevValidationResult = el.__prevValidationResult; + const prevShownValidationFeedback = el.__prevShownValidationFeedback; const regularValidationResult = [ ...el.__syncValidationResult, ...el.__asyncValidationResult, ]; expect(resultValidateSpy.args[0][0]).to.eql({ - prevValidationResult, regularValidationResult, + prevShownValidationFeedback, }); }); @@ -1140,7 +1146,7 @@ export function runValidateMixinSuite(customConfig) { >${lightDom} `)); - const spy = sinon.spy(el, '_updateFeedbackComponent'); + const spy = sinon.spy(el, '_updateShouldShowFeedbackFor'); let counter = 0; // for ... of is already allowed we should update eslint... // eslint-disable-next-line no-restricted-syntax diff --git a/packages/form-core/test/validate/ResultValidator.test.js b/packages/form-core/test/validate/ResultValidator.test.js index 2fdfe13f1..db98f7f54 100644 --- a/packages/form-core/test/validate/ResultValidator.test.js +++ b/packages/form-core/test/validate/ResultValidator.test.js @@ -15,18 +15,19 @@ describe('ResultValidator', () => { * * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {Validator[]} context.prevValidationResult + * @param {string} context.prevShownValidationFeedback * @returns {boolean} */ - executeOnResults({ regularValidationResult, prevValidationResult }) { - const hasSuccess = regularValidationResult.length && !prevValidationResult.length; + executeOnResults({ regularValidationResult, prevShownValidationFeedback }) { + const hasSuccess = + regularValidationResult.length && prevShownValidationFeedback === 'error'; return !!hasSuccess; } } expect( new MyResultValidator().executeOnResults({ regularValidationResult: [new Required(), new MinLength(3)], - prevValidationResult: [], + prevShownValidationFeedback: 'error', }), ).to.be.true; }); diff --git a/packages/form-core/types/validate/ValidateMixinTypes.d.ts b/packages/form-core/types/validate/ValidateMixinTypes.d.ts index a5b16201a..7b6a93788 100644 --- a/packages/form-core/types/validate/ValidateMixinTypes.d.ts +++ b/packages/form-core/types/validate/ValidateMixinTypes.d.ts @@ -37,7 +37,7 @@ export declare class ValidateHost { __syncValidationResult: Validator[]; __asyncValidationResult: Validator[]; __validationResult: Validator[]; - __prevValidationResult: Validator[]; + __prevShownValidationFeedback: string; connectedCallback(): void; disconnectedCallback(): void; diff --git a/packages/form-integrations/test/form-validation-integrations.test.js b/packages/form-integrations/test/form-validation-integrations.test.js new file mode 100644 index 000000000..f662f5b3d --- /dev/null +++ b/packages/form-integrations/test/form-validation-integrations.test.js @@ -0,0 +1,114 @@ +import { expect, fixture, html, defineCE, unsafeStatic } from '@open-wc/testing'; +import { Required, DefaultSuccess, Validator } from '@lion/form-core'; +import { loadDefaultFeedbackMessages } from '@lion/validate-messages'; +import { LionInput } from '@lion/input'; +import sinon from 'sinon'; + +describe('Form Validation Integrations', () => { + const lightDom = ''; + before(() => { + loadDefaultFeedbackMessages(); + }); + + describe('DefaultSuccess', () => { + it('does not show success feedback if the user is not recovering from a shown error/warning feedback', async () => { + class WarnValidator extends Validator { + /** + * + * @param {?} [param] + * @param {Object.} [config] + */ + constructor(param, config) { + super(param, config); + this.type = 'warning'; + } + + /** @param {string} value */ + execute(value) { + let hasError = false; + if (value === 'warn') { + hasError = true; + } + return hasError; + } + } + + class ValidateElementCustomTypes extends LionInput { + static get validationTypes() { + return ['error', 'warning', 'success']; + } + } + const elTagString = defineCE(ValidateElementCustomTypes); + const elTag = unsafeStatic(elTagString); + const el = /** @type {ValidateElementCustomTypes} */ (await fixture(html` + <${elTag} + .validators=${[ + new Required(null, { getMessage: () => 'error' }), + new WarnValidator(null, { getMessage: () => 'warning' }), + new DefaultSuccess(), + ]} + >${lightDom} + `)); + + expect(el._feedbackNode.feedbackData?.length).to.equal(0); + + el.modelValue = 'w'; + el.touched = true; + await el.updateComplete; + await el.feedbackComplete; + expect(el.showsFeedbackFor).to.eql([]); + + el.modelValue = 'warn'; + await el.updateComplete; + await el.feedbackComplete; + expect(el._feedbackNode.feedbackData?.[0].message).to.equal('warning'); + + el.modelValue = 'war'; + await el.updateComplete; + await el.feedbackComplete; + + expect([ + 'Okay', + 'Correct', + 'Succeeded', + 'Ok!', + 'This is right.', + 'Changed!', + 'Ok, correct.', + ]).to.include( + /** @type {{ message: string ;type: string; validator?: Validator | undefined;}[]} */ (el + ._feedbackNode.feedbackData)[0].message, + ); + + el.modelValue = ''; + await el.updateComplete; + await el.feedbackComplete; + expect(el._feedbackNode.feedbackData?.[0].message).to.equal('error'); + + el.modelValue = 'war'; + await el.updateComplete; + await el.feedbackComplete; + + expect([ + 'Okay', + 'Correct', + 'Succeeded', + 'Ok!', + 'This is right.', + 'Changed!', + 'Ok, correct.', + ]).to.include( + /** @type {{ message: string ;type: string; validator?: Validator | undefined;}[]} */ (el + ._feedbackNode.feedbackData)[0].message, + ); + + // Check that change in focused or other interaction states does not refresh the success message + // without a change in validation results + const spy = sinon.spy(el, '_updateFeedbackComponent'); + el._updateShouldShowFeedbackFor(); + await el.updateComplete; + await el.feedbackComplete; + expect(spy.called).to.be.false; + }); + }); +}); From 1d9772176933f3283ccf7ceaafcf217cfca72f02 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 15 Mar 2021 10:46:08 +0100 Subject: [PATCH 2/2] chore: revert prevValidation result, make prevShown Validator[] --- .eslintignore | 1 + .../form-core/src/validate/ResultValidator.js | 14 ++++++++--- .../form-core/src/validate/ValidateMixin.js | 12 ++++++--- .../resultValidators/DefaultSuccess.js | 11 ++++---- .../test-suites/ValidateMixin.suite.js | 17 ++++++------- .../test/validate/ResultValidator.test.js | 25 ++++++++++++++----- .../types/validate/ValidateMixinTypes.d.ts | 3 ++- 7 files changed, 54 insertions(+), 29 deletions(-) diff --git a/.eslintignore b/.eslintignore index 9a1a5c81d..83876c388 100644 --- a/.eslintignore +++ b/.eslintignore @@ -2,6 +2,7 @@ node_modules coverage/ bundlesize/ .history/ +storybook-static/ *.d.ts _site-dev _site diff --git a/packages/form-core/src/validate/ResultValidator.js b/packages/form-core/src/validate/ResultValidator.js index 5b4362339..4588e771b 100644 --- a/packages/form-core/src/validate/ResultValidator.js +++ b/packages/form-core/src/validate/ResultValidator.js @@ -10,12 +10,20 @@ export class ResultValidator extends Validator { /** * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {string} context.prevShownValidationFeedback + * @param {Validator[]} context.prevValidationResult + * @param {Validator[]} context.prevShownValidationResult * @param {Validator[]} [context.validators] * @returns {boolean} */ - // eslint-disable-next-line no-unused-vars, class-methods-use-this - executeOnResults({ regularValidationResult, prevShownValidationFeedback, validators }) { + /* eslint-disable no-unused-vars */ + // eslint-disable-next-line class-methods-use-this + executeOnResults({ + regularValidationResult, + prevValidationResult, + prevShownValidationResult, + validators, + }) { + /* eslint-enable no-unused-vars */ return true; } } diff --git a/packages/form-core/src/validate/ValidateMixin.js b/packages/form-core/src/validate/ValidateMixin.js index 66ba3f5d4..8bb300ce6 100644 --- a/packages/form-core/src/validate/ValidateMixin.js +++ b/packages/form-core/src/validate/ValidateMixin.js @@ -161,8 +161,10 @@ export const ValidateMixinImplementation = superclass => * @type {Validator[]} */ this.__validationResult = []; - /** @type {string} */ - this.__prevShownValidationFeedback = ''; + /** @type {Validator[]} */ + this.__prevValidationResult = []; + /** @type {Validator[]} */ + this.__prevShownValidationResult = []; this.__onValidatorUpdated = this.__onValidatorUpdated.bind(this); this._updateFeedbackComponent = this._updateFeedbackComponent.bind(this); @@ -279,6 +281,7 @@ export const ValidateMixinImplementation = superclass => return; } + this.__prevValidationResult = this.__validationResult; if (clearCurrentResult) { // Clear ('invalidate') all pending and existing validation results. // This is needed because we have async (pending) validators whose results @@ -396,7 +399,8 @@ export const ValidateMixinImplementation = superclass => return resultValidators.filter(v => v.executeOnResults({ regularValidationResult, - prevShownValidationFeedback: this.__prevShownValidationFeedback, + prevValidationResult: this.__prevValidationResult, + prevShownValidationResult: this.__prevShownValidationResult, }), ); } @@ -579,7 +583,7 @@ export const ValidateMixinImplementation = superclass => }); if (this.__prioritizedResult.length > 0) { - this.__prevShownValidationFeedback = this.__prioritizedResult[0].type; + this.__prevShownValidationResult = this.__prioritizedResult; } const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult); diff --git a/packages/form-core/src/validate/resultValidators/DefaultSuccess.js b/packages/form-core/src/validate/resultValidators/DefaultSuccess.js index ad3a9c7dc..b2b9e35c3 100644 --- a/packages/form-core/src/validate/resultValidators/DefaultSuccess.js +++ b/packages/form-core/src/validate/resultValidators/DefaultSuccess.js @@ -17,18 +17,17 @@ export class DefaultSuccess extends ResultValidator { * * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {string} context.prevShownValidationFeedback + * @param {Validator[]} context.prevValidationResult + * @param {Validator[]} context.prevShownValidationResult * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this - executeOnResults({ regularValidationResult, prevShownValidationFeedback }) { + executeOnResults({ regularValidationResult, prevShownValidationResult }) { const errorOrWarning = /** @param {Validator} v */ v => v.type === 'error' || v.type === 'warning'; const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length; + const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length; - return ( - !hasErrorOrWarning && - (prevShownValidationFeedback === 'error' || prevShownValidationFeedback === 'warning') - ); + return !hasErrorOrWarning && hasShownErrorOrWarning; } } diff --git a/packages/form-core/test-suites/ValidateMixin.suite.js b/packages/form-core/test-suites/ValidateMixin.suite.js index d27795931..73e044f0e 100644 --- a/packages/form-core/test-suites/ValidateMixin.suite.js +++ b/packages/form-core/test-suites/ValidateMixin.suite.js @@ -550,19 +550,16 @@ export function runValidateMixinSuite(customConfig) { * * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {string} context.prevShownValidationFeedback + * @param {Validator[]} context.prevShownValidationResult * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this - executeOnResults({ regularValidationResult, prevShownValidationFeedback }) { + executeOnResults({ regularValidationResult, prevShownValidationResult }) { const errorOrWarning = /** @param {Validator} v */ v => v.type === 'error' || v.type === 'warning'; const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length; - - return ( - !hasErrorOrWarning && - (prevShownValidationFeedback === 'error' || prevShownValidationFeedback === 'warning') - ); + const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length; + return !hasErrorOrWarning && hasShownErrorOrWarning; } }; @@ -613,7 +610,8 @@ export function runValidateMixinSuite(customConfig) { .modelValue=${'myValue'} >${lightDom} `)); - const prevShownValidationFeedback = el.__prevShownValidationFeedback; + const prevValidationResult = el.__prevValidationResult; + const prevShownValidationResult = el.__prevShownValidationResult; const regularValidationResult = [ ...el.__syncValidationResult, ...el.__asyncValidationResult, @@ -621,7 +619,8 @@ export function runValidateMixinSuite(customConfig) { expect(resultValidateSpy.args[0][0]).to.eql({ regularValidationResult, - prevShownValidationFeedback, + prevValidationResult, + prevShownValidationResult, }); }); diff --git a/packages/form-core/test/validate/ResultValidator.test.js b/packages/form-core/test/validate/ResultValidator.test.js index db98f7f54..a933eac27 100644 --- a/packages/form-core/test/validate/ResultValidator.test.js +++ b/packages/form-core/test/validate/ResultValidator.test.js @@ -1,5 +1,6 @@ import { expect } from '@open-wc/testing'; import { ResultValidator } from '../../src/validate/ResultValidator.js'; +import { DefaultSuccess } from '../../src/validate/resultValidators/DefaultSuccess.js'; import { Required } from '../../src/validate/validators/Required.js'; import { MinLength } from '../../src/validate/validators/StringValidators.js'; @@ -15,19 +16,31 @@ describe('ResultValidator', () => { * * @param {Object} context * @param {Validator[]} context.regularValidationResult - * @param {string} context.prevShownValidationFeedback + * @param {Validator[]} context.prevValidationResult + * @param {Validator[]} context.prevShownValidationResult * @returns {boolean} */ - executeOnResults({ regularValidationResult, prevShownValidationFeedback }) { - const hasSuccess = - regularValidationResult.length && prevShownValidationFeedback === 'error'; - return !!hasSuccess; + executeOnResults({ regularValidationResult, prevShownValidationResult }) { + const errorOrWarning = /** @param {Validator} v */ v => + v.type === 'error' || v.type === 'warning'; + const hasErrorOrWarning = !!regularValidationResult.filter(errorOrWarning).length; + const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length; + + return !hasErrorOrWarning && hasShownErrorOrWarning; } } expect( new MyResultValidator().executeOnResults({ regularValidationResult: [new Required(), new MinLength(3)], - prevShownValidationFeedback: 'error', + prevValidationResult: [], + prevShownValidationResult: [], + }), + ).to.be.false; + expect( + new MyResultValidator().executeOnResults({ + regularValidationResult: [new DefaultSuccess()], + prevValidationResult: [new Required()], + prevShownValidationResult: [new Required()], }), ).to.be.true; }); diff --git a/packages/form-core/types/validate/ValidateMixinTypes.d.ts b/packages/form-core/types/validate/ValidateMixinTypes.d.ts index 7b6a93788..56a567b72 100644 --- a/packages/form-core/types/validate/ValidateMixinTypes.d.ts +++ b/packages/form-core/types/validate/ValidateMixinTypes.d.ts @@ -37,7 +37,8 @@ export declare class ValidateHost { __syncValidationResult: Validator[]; __asyncValidationResult: Validator[]; __validationResult: Validator[]; - __prevShownValidationFeedback: string; + __prevValidationResult: Validator[]; + __prevShownValidationResult: Validator[]; connectedCallback(): void; disconnectedCallback(): void;