From 247e64a3cc1522960b3b206026008a495d83fbe3 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 12 Oct 2020 12:26:30 +0200 Subject: [PATCH 1/3] fix(form-core): update child choice fields name attr to parent --- .changeset/sour-apes-reply.md | 5 +++ .../src/choice-group/ChoiceGroupMixin.js | 32 ++++++++----------- .../choice-group/ChoiceGroupMixin.suite.js | 32 ++++++++++++++----- .../listbox/test-suites/ListboxMixin.suite.js | 18 ----------- 4 files changed, 42 insertions(+), 45 deletions(-) create mode 100644 .changeset/sour-apes-reply.md diff --git a/.changeset/sour-apes-reply.md b/.changeset/sour-apes-reply.md new file mode 100644 index 000000000..4c624a470 --- /dev/null +++ b/.changeset/sour-apes-reply.md @@ -0,0 +1,5 @@ +--- +'@lion/form-core': patch +--- + +Ensure that the name of a child choice field is always synced with the parent choice field(set) when it changes. No longer error when a child is added with a different name than the parent, simply sync it. diff --git a/packages/form-core/src/choice-group/ChoiceGroupMixin.js b/packages/form-core/src/choice-group/ChoiceGroupMixin.js index d9839632a..0d4fe415a 100644 --- a/packages/form-core/src/choice-group/ChoiceGroupMixin.js +++ b/packages/form-core/src/choice-group/ChoiceGroupMixin.js @@ -156,6 +156,17 @@ const ChoiceGroupMixinImplementation = superclass => }); } + /** @param {import('lit-element').PropertyValues} changedProperties */ + updated(changedProperties) { + super.updated(changedProperties); + if (changedProperties.has('name') && this.name !== changedProperties.get('name')) { + this.formElements.forEach(child => { + // eslint-disable-next-line no-param-reassign + child.name = this.name; + }); + } + } + disconnectedCallback() { super.disconnectedCallback(); @@ -171,7 +182,8 @@ const ChoiceGroupMixinImplementation = superclass => */ addFormElement(child, indexToInsertAt) { this._throwWhenInvalidChildModelValue(child); - this.__delegateNameAttribute(child); + // eslint-disable-next-line no-param-reassign + child.name = this.name; super.addFormElement(child, indexToInsertAt); } @@ -283,24 +295,6 @@ const ChoiceGroupMixinImplementation = superclass => } } - /** - * @param {FormControl} child - */ - __delegateNameAttribute(child) { - if (!child.name || child.name === this.name) { - // eslint-disable-next-line no-param-reassign - child.name = this.name; - } else { - throw new Error( - `The ${this.tagName.toLowerCase()} name="${ - this.name - }" does not allow to register ${child.tagName.toLowerCase()} with custom names (name="${ - child.name - }" given)`, - ); - } - } - /** * @override FormControlMixin * @param {CustomEvent} ev diff --git a/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js b/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js index 60464c0e8..ec118c3b8 100644 --- a/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js +++ b/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js @@ -74,7 +74,7 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = { ); }); - it('automatically sets the name property of child radios to its own name', async () => { + it('automatically sets the name property of child fields to its own name', async () => { const el = /** @type {ChoiceGroup} */ (await fixture(html` <${parentTag} name="gender"> <${childTag} .choiceValue=${'female'} checked> @@ -93,7 +93,26 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = { expect(el.formElements[2].name).to.equal('gender'); }); - it('throws if a child element with a different name than the group tries to register', async () => { + it('automatically updates the name property of child fields to its own name', async () => { + const el = /** @type {ChoiceGroup} */ (await fixture(html` + <${parentTag} name="gender"> + <${childTag}> + <${childTag}> + + `)); + + expect(el.formElements[0].name).to.equal('gender'); + expect(el.formElements[1].name).to.equal('gender'); + + el.name = 'gender2'; + + await el.updateComplete; + + expect(el.formElements[0].name).to.equal('gender2'); + expect(el.formElements[1].name).to.equal('gender2'); + }); + + it('adjusts the name of a child element if it has a different name than the group', async () => { const el = /** @type {ChoiceGroup} */ (await fixture(html` <${parentTag} name="gender"> <${childTag} .choiceValue=${'female'} checked> @@ -104,12 +123,9 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = { const invalidChild = /** @type {ChoiceGroup} */ (await fixture(html` <${childTag} name="foo" .choiceValue=${'male'}> `)); - - expect(() => { - el.addFormElement(invalidChild); - }).to.throw( - 'The choice-group name="gender" does not allow to register choice-group-input with custom names (name="foo" given)', - ); + el.addFormElement(invalidChild); + await invalidChild.updateComplete; + expect(invalidChild.name).to.equal('gender'); }); it('can set initial modelValue on creation', async () => { diff --git a/packages/listbox/test-suites/ListboxMixin.suite.js b/packages/listbox/test-suites/ListboxMixin.suite.js index 8a230ffd7..858ca2a51 100644 --- a/packages/listbox/test-suites/ListboxMixin.suite.js +++ b/packages/listbox/test-suites/ListboxMixin.suite.js @@ -96,24 +96,6 @@ export function runListboxMixinSuite(customConfig = {}) { ); }); - it('throws if a child element with a different name than the group tries to register', async () => { - const el = await fixture(html` - <${tag} name="gender"> - <${optionTag} .choiceValue=${'female'} checked> - <${optionTag} .choiceValue=${'other'}> - - `); - const invalidChild = await fixture(html` - <${optionTag} name="foo" .choiceValue=${'male'}> - `); - - expect(() => { - el.addFormElement(invalidChild); - }).to.throw( - `The ${cfg.tagString} name="gender" does not allow to register lion-option with custom names (name="foo" given)`, - ); - }); - it('can set initial modelValue on creation', async () => { const el = await fixture(html` <${tag} name="gender" .modelValue=${'other'}> From fd297a2832d7bb89668eacde0000a62789d6f293 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 12 Oct 2020 14:13:05 +0200 Subject: [PATCH 2/3] fix(form-core): update formElements with the right name on change --- .changeset/hungry-files-lay.md | 5 ++++ packages/form-core/src/FormControlMixin.js | 16 ++++++++++--- .../src/choice-group/ChoiceGroupMixin.js | 2 +- .../src/registration/FormRegistrarMixin.js | 16 +++++++++++++ .../ValidateMixinFeedbackPart.suite.js | 4 ++-- .../form-group/FormGroupMixin.suite.js | 24 +++++++++++++++---- .../types/FormControlMixinTypes.d.ts | 4 ++-- .../types/form-group/FormGroupMixinTypes.d.ts | 2 +- 8 files changed, 59 insertions(+), 14 deletions(-) create mode 100644 .changeset/hungry-files-lay.md diff --git a/.changeset/hungry-files-lay.md b/.changeset/hungry-files-lay.md new file mode 100644 index 000000000..c720f0d2f --- /dev/null +++ b/.changeset/hungry-files-lay.md @@ -0,0 +1,5 @@ +--- +'@lion/form-core': patch +--- + +Properly update formElements when the name attribute changes, in order to get an updated serializedValue. diff --git a/packages/form-core/src/FormControlMixin.js b/packages/form-core/src/FormControlMixin.js index b764a22cc..2488c650c 100644 --- a/packages/form-core/src/FormControlMixin.js +++ b/packages/form-core/src/FormControlMixin.js @@ -23,8 +23,8 @@ function uuid(prefix) { * @typedef {import('@lion/core').nothing} nothing * @typedef {import('@lion/core/types/SlotMixinTypes').SlotsMap} SlotsMap * @typedef {import('../types/FormControlMixinTypes.js').FormControlMixin} FormControlMixin - * @type {FormControlMixin} * @param {import('@open-wc/dedupe-mixin').Constructor} superclass + * @type {FormControlMixin} */ const FormControlMixinImplementation = superclass => // eslint-disable-next-line no-shadow, no-unused-vars @@ -144,8 +144,7 @@ const FormControlMixinImplementation = superclass => * @return {string} */ get fieldName() { - // @ts-expect-error - return this.__fieldName || this.label || this.name; // FIXME: when LionField is typed we can inherit this prop + return this.__fieldName || this.label || this.name || ''; } /** @@ -195,6 +194,8 @@ const FormControlMixinImplementation = superclass => constructor() { super(); + /** @type {string | undefined} */ + this.name = undefined; /** @type {string} */ this._inputId = uuid(this.localName); /** @type {HTMLElement[]} */ @@ -257,6 +258,15 @@ const FormControlMixinImplementation = superclass => if (changedProperties.has('helpText') && this._helpTextNode) { this._helpTextNode.textContent = this.helpText; } + + if (changedProperties.has('name')) { + this.dispatchEvent( + new CustomEvent('form-element-name-changed', { + detail: { oldName: changedProperties.get('name'), newName: this.name }, + bubbles: true, + }), + ); + } } _triggerInitialModelValueChangedEvent() { diff --git a/packages/form-core/src/choice-group/ChoiceGroupMixin.js b/packages/form-core/src/choice-group/ChoiceGroupMixin.js index 0d4fe415a..2212a0676 100644 --- a/packages/form-core/src/choice-group/ChoiceGroupMixin.js +++ b/packages/form-core/src/choice-group/ChoiceGroupMixin.js @@ -15,7 +15,6 @@ import { InteractionStateMixin } from '../InteractionStateMixin.js'; * @param {import('@open-wc/dedupe-mixin').Constructor} superclass */ const ChoiceGroupMixinImplementation = superclass => - // @ts-expect-error class ChoiceGroupMixin extends FormRegistrarMixin(InteractionStateMixin(superclass)) { static get properties() { return { @@ -121,6 +120,7 @@ const ChoiceGroupMixinImplementation = superclass => constructor() { super(); this.multipleChoice = false; + /** @type {'child'|'choice-group'|'fieldset'} */ this._repropagationRole = 'choice-group'; // configures event propagation logic of FormControlMixin this.__isInitialModelValue = true; diff --git a/packages/form-core/src/registration/FormRegistrarMixin.js b/packages/form-core/src/registration/FormRegistrarMixin.js index c28f03aad..74d7717fb 100644 --- a/packages/form-core/src/registration/FormRegistrarMixin.js +++ b/packages/form-core/src/registration/FormRegistrarMixin.js @@ -50,11 +50,16 @@ const FormRegistrarMixinImplementation = superclass => this._isFormOrFieldset = false; this._onRequestToAddFormElement = this._onRequestToAddFormElement.bind(this); + this._onRequestToChangeFormElementName = this._onRequestToChangeFormElementName.bind(this); this.addEventListener( 'form-element-register', /** @type {EventListenerOrEventListenerObject} */ (this._onRequestToAddFormElement), ); + this.addEventListener( + 'form-element-name-changed', + /** @type {EventListenerOrEventListenerObject} */ (this._onRequestToChangeFormElementName), + ); } /** @@ -163,6 +168,17 @@ const FormRegistrarMixinImplementation = superclass => this.addFormElement(child, indexToInsertAt); } + /** + * @param {CustomEvent} ev + */ + _onRequestToChangeFormElementName(ev) { + const element = this.formElements[ev.detail.oldName]; + if (element) { + this.formElements[ev.detail.newName] = element; + delete this.formElements[ev.detail.oldName]; + } + } + /** * @param {CustomEvent} ev */ diff --git a/packages/form-core/test-suites/ValidateMixinFeedbackPart.suite.js b/packages/form-core/test-suites/ValidateMixinFeedbackPart.suite.js index a634a98ed..e4fc39e0b 100644 --- a/packages/form-core/test-suites/ValidateMixinFeedbackPart.suite.js +++ b/packages/form-core/test-suites/ValidateMixinFeedbackPart.suite.js @@ -390,7 +390,7 @@ export function runValidateMixinFeedbackPart() { params: 4, modelValue: 'cat', formControl: el, - fieldName: undefined, + fieldName: '', type: 'x', name: 'MinLength', }); @@ -414,7 +414,7 @@ export function runValidateMixinFeedbackPart() { params: 4, modelValue: 'cat', formControl: el, - fieldName: undefined, + fieldName: '', type: 'error', name: 'MinLength', }); diff --git a/packages/form-core/test-suites/form-group/FormGroupMixin.suite.js b/packages/form-core/test-suites/form-group/FormGroupMixin.suite.js index dbe8451cc..d2a681723 100644 --- a/packages/form-core/test-suites/form-group/FormGroupMixin.suite.js +++ b/packages/form-core/test-suites/form-group/FormGroupMixin.suite.js @@ -18,26 +18,27 @@ import { FormGroupMixin } from '../../src/form-group/FormGroupMixin.js'; * @param {{ tagString?: string, childTagString?:string }} [cfg] */ export function runFormGroupMixinSuite(cfg = {}) { - const FormChild = class extends LionField { + class FormChild extends LionField { get slots() { return { ...super.slots, input: () => document.createElement('input'), }; } - }; + } const childTagString = cfg.childTagString || defineCE(FormChild); - // @ts-expect-error - const FormGroup = class extends FormGroupMixin(LitElement) { + // @ts-expect-error base constructors same return type + class FormGroup extends FormGroupMixin(LitElement) { constructor() { super(); /** @override from FormRegistrarMixin */ this._isFormOrFieldset = true; + /** @type {'child'|'choice-group'|'fieldset'} */ this._repropagationRole = 'fieldset'; // configures FormControlMixin } - }; + } const tagString = cfg.tagString || defineCE(FormGroup); const tag = unsafeStatic(tagString); @@ -804,6 +805,19 @@ export function runFormGroupMixinSuite(cfg = {}) { }, }); }); + + it('updates the formElements keys when a name attribute changes', async () => { + const fieldset = /** @type {FormGroup} */ (await fixture(html` + <${tag}> + <${childTag} name="foo" .modelValue=${'qux'}> + + `)); + expect(fieldset.serializedValue.foo).to.equal('qux'); + fieldset.formElements[0].name = 'bar'; + await fieldset.updateComplete; + await fieldset.formElements[0].updateComplete; + expect(fieldset.serializedValue.bar).to.equal('qux'); + }); }); describe('Reset', () => { diff --git a/packages/form-core/types/FormControlMixinTypes.d.ts b/packages/form-core/types/FormControlMixinTypes.d.ts index 6af5a4e5f..0a6095597 100644 --- a/packages/form-core/types/FormControlMixinTypes.d.ts +++ b/packages/form-core/types/FormControlMixinTypes.d.ts @@ -10,7 +10,7 @@ declare interface HTMLElementWithValue extends HTMLElement { value: string; } -export class FormControlHost { +export declare class FormControlHost { static get styles(): CSSResult | CSSResult[]; /** * A Boolean attribute which, if present, indicates that the user should not be able to edit @@ -129,7 +129,7 @@ export declare function FormControlImplementation & - FormControlHost & + typeof FormControlHost & Constructor & typeof FormRegisteringHost & Constructor & diff --git a/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts b/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts index f27a6ba55..7afaa1c4d 100644 --- a/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts +++ b/packages/form-core/types/form-group/FormGroupMixinTypes.d.ts @@ -15,7 +15,7 @@ export declare class FormGroupHost { touched: boolean; dirty: boolean; submitted: boolean; - serializedValue: string; + serializedValue: { [key: string]: any }; modelValue: { [x: string]: any }; formattedValue: string; children: Array; From e25586e6061814e0b1f323b8e9caf4dc08af2071 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 12 Oct 2020 14:23:51 +0200 Subject: [PATCH 3/3] fix(form-core): prevent choice input from changing name away from parent --- .../src/choice-group/ChoiceInputMixin.js | 11 +++++++++++ .../choice-group/ChoiceGroupMixin.suite.js | 17 +++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/packages/form-core/src/choice-group/ChoiceInputMixin.js b/packages/form-core/src/choice-group/ChoiceInputMixin.js index 5c3694864..931ded641 100644 --- a/packages/form-core/src/choice-group/ChoiceInputMixin.js +++ b/packages/form-core/src/choice-group/ChoiceInputMixin.js @@ -110,6 +110,17 @@ const ChoiceInputMixinImplementation = superclass => if (changedProperties.has('modelValue')) { this.__syncCheckedToInputElement(); } + + if ( + changedProperties.has('name') && + // @ts-expect-error not all choice inputs have a parent form group, since this mixin does not have a strict contract with the registration system + this.__parentFormGroup && + // @ts-expect-error + this.__parentFormGroup.name !== this.name + ) { + // @ts-expect-error not all choice inputs have a name prop, because this mixin does not have a strict contract with form control mixin + this.name = changedProperties.get('name'); + } } constructor() { diff --git a/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js b/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js index ec118c3b8..d250f7b5f 100644 --- a/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js +++ b/packages/form-core/test-suites/choice-group/ChoiceGroupMixin.suite.js @@ -112,6 +112,23 @@ export function runChoiceGroupMixinSuite({ parentTagString, childTagString } = { expect(el.formElements[1].name).to.equal('gender2'); }); + it('prevents updating the name property of a child if it is different from its parent', async () => { + const el = /** @type {ChoiceGroup} */ (await fixture(html` + <${parentTag} name="gender"> + <${childTag}> + <${childTag}> + + `)); + + expect(el.formElements[0].name).to.equal('gender'); + expect(el.formElements[1].name).to.equal('gender'); + + el.formElements[0].name = 'gender2'; + + await el.formElements[0].updateComplete; + expect(el.formElements[0].name).to.equal('gender'); + }); + it('adjusts the name of a child element if it has a different name than the group', async () => { const el = /** @type {ChoiceGroup} */ (await fixture(html` <${parentTag} name="gender">