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/.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 014eaf290..4588e771b 100644 --- a/packages/form-core/src/validate/ResultValidator.js +++ b/packages/form-core/src/validate/ResultValidator.js @@ -11,11 +11,19 @@ export class ResultValidator extends Validator { * @param {Object} context * @param {Validator[]} context.regularValidationResult * @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, prevValidationResult, 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 9fa844b61..8bb300ce6 100644 --- a/packages/form-core/src/validate/ValidateMixin.js +++ b/packages/form-core/src/validate/ValidateMixin.js @@ -163,6 +163,8 @@ export const ValidateMixinImplementation = superclass => this.__validationResult = []; /** @type {Validator[]} */ this.__prevValidationResult = []; + /** @type {Validator[]} */ + this.__prevShownValidationResult = []; this.__onValidatorUpdated = this.__onValidatorUpdated.bind(this); this._updateFeedbackComponent = this._updateFeedbackComponent.bind(this); @@ -279,7 +281,7 @@ export const ValidateMixinImplementation = superclass => return; } - this.__storePrevResult(); + 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 @@ -289,10 +291,6 @@ export const ValidateMixinImplementation = superclass => await this.__executeValidators(); } - __storePrevResult() { - this.__prevValidationResult = this.__validationResult; - } - /** * @desc step A1-3 + B (as explained in 'validate') */ @@ -402,6 +400,7 @@ export const ValidateMixinImplementation = superclass => v.executeOnResults({ regularValidationResult, prevValidationResult: this.__prevValidationResult, + prevShownValidationResult: this.__prevShownValidationResult, }), ); } @@ -582,8 +581,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.__prevShownValidationResult = this.__prioritizedResult; + } + + const messageMap = await this.__getFeedbackMessages(this.__prioritizedResult); _feedbackNode.feedbackData = messageMap.length ? messageMap : []; }); } else { @@ -636,10 +639,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..b2b9e35c3 100644 --- a/packages/form-core/src/validate/resultValidators/DefaultSuccess.js +++ b/packages/form-core/src/validate/resultValidators/DefaultSuccess.js @@ -18,14 +18,16 @@ export class DefaultSuccess extends ResultValidator { * @param {Object} context * @param {Validator[]} context.regularValidationResult * @param {Validator[]} context.prevValidationResult + * @param {Validator[]} context.prevShownValidationResult * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this - executeOnResults({ regularValidationResult, prevValidationResult }) { + executeOnResults({ regularValidationResult, prevShownValidationResult }) { 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; + const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length; + + return !hasErrorOrWarning && hasShownErrorOrWarning; } } diff --git a/packages/form-core/test-suites/ValidateMixin.suite.js b/packages/form-core/test-suites/ValidateMixin.suite.js index b6973ea08..73e044f0e 100644 --- a/packages/form-core/test-suites/ValidateMixin.suite.js +++ b/packages/form-core/test-suites/ValidateMixin.suite.js @@ -548,15 +548,18 @@ export function runValidateMixinSuite(customConfig) { /** * - * @param {{ regularValidationResult: Validator[], prevValidationResult: Validator[]}} param0 + * @param {Object} context + * @param {Validator[]} context.regularValidationResult + * @param {Validator[]} context.prevShownValidationResult + * @returns {boolean} */ // eslint-disable-next-line class-methods-use-this - executeOnResults({ regularValidationResult, prevValidationResult }) { + executeOnResults({ regularValidationResult, prevShownValidationResult }) { 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; + const hasShownErrorOrWarning = !!prevShownValidationResult.filter(errorOrWarning).length; + return !hasErrorOrWarning && hasShownErrorOrWarning; } }; @@ -608,14 +611,16 @@ export function runValidateMixinSuite(customConfig) { >${lightDom} `)); const prevValidationResult = el.__prevValidationResult; + const prevShownValidationResult = el.__prevShownValidationResult; const regularValidationResult = [ ...el.__syncValidationResult, ...el.__asyncValidationResult, ]; expect(resultValidateSpy.args[0][0]).to.eql({ - prevValidationResult, regularValidationResult, + prevValidationResult, + prevShownValidationResult, }); }); @@ -1140,7 +1145,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..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'; @@ -16,17 +17,30 @@ describe('ResultValidator', () => { * @param {Object} context * @param {Validator[]} context.regularValidationResult * @param {Validator[]} context.prevValidationResult + * @param {Validator[]} context.prevShownValidationResult * @returns {boolean} */ - executeOnResults({ regularValidationResult, prevValidationResult }) { - const hasSuccess = regularValidationResult.length && !prevValidationResult.length; - 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)], 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 a5b16201a..56a567b72 100644 --- a/packages/form-core/types/validate/ValidateMixinTypes.d.ts +++ b/packages/form-core/types/validate/ValidateMixinTypes.d.ts @@ -38,6 +38,7 @@ export declare class ValidateHost { __asyncValidationResult: Validator[]; __validationResult: Validator[]; __prevValidationResult: Validator[]; + __prevShownValidationResult: Validator[]; 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; + }); + }); +});