From 9fcb67f0ec85cd325072c649cd02fb4543df1458 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 12 Oct 2020 18:29:45 +0200 Subject: [PATCH] fix(combobox): dispatch model-value-changed properly on unselect --- .changeset/silent-waves-teach.md | 8 +++++ packages/combobox/README.md | 30 +++++++++++++++++++ packages/combobox/package.json | 3 ++ packages/combobox/src/LionCombobox.js | 23 ++++++++++++-- packages/combobox/test/lion-combobox.test.js | 26 ++++++++++++++++ packages/form-core/src/FormControlMixin.js | 21 +++++++++++-- .../docs/15-features-overview.md | 7 ++++- packages/listbox/src/ListboxMixin.js | 8 ++++- 8 files changed, 119 insertions(+), 7 deletions(-) create mode 100644 .changeset/silent-waves-teach.md diff --git a/.changeset/silent-waves-teach.md b/.changeset/silent-waves-teach.md new file mode 100644 index 000000000..7c0f7953a --- /dev/null +++ b/.changeset/silent-waves-teach.md @@ -0,0 +1,8 @@ +--- +'@lion/combobox': patch +'@lion/form-core': patch +'@lion/form-integrations': patch +'@lion/listbox': patch +--- + +Allow flexibility for extending the repropagation prevention conditions, which is needed for combobox, so that a model-value-changed event is propagated when no option matches after an input change. This allows validation to work properly e.g. for Required. diff --git a/packages/combobox/README.md b/packages/combobox/README.md index b5210901b..56736707f 100644 --- a/packages/combobox/README.md +++ b/packages/combobox/README.md @@ -263,6 +263,36 @@ export const invokerButton = () => html` `; ``` +## Validation + +Validation can be used as normal, below is an example of a combobox with a `Required` validator. + +```js story +export const validation = () => { + loadDefaultFeedbackMessages(); + Required.getMessage = () => 'Please enter a value'; + return html` + +
+ + Rocky + Rocky II + Rocky III + Rocky IV + Rocky V + Rocky Balboa + +
+ + `; +}; +``` + ## Listbox compatibility All configurations that can be applied to `lion-listbox`, can be applied to `lion-combobox` as well. diff --git a/packages/combobox/package.json b/packages/combobox/package.json index 96f1dd0e6..dbcf07604 100644 --- a/packages/combobox/package.json +++ b/packages/combobox/package.json @@ -46,6 +46,9 @@ "@lion/listbox": "0.2.0", "@lion/overlays": "0.21.0" }, + "devDependencies": { + "@lion/validate-messages": "0.3.1" + }, "keywords": [ "combobox", "form", diff --git a/packages/combobox/src/LionCombobox.js b/packages/combobox/src/LionCombobox.js index d66e33750..02566c574 100644 --- a/packages/combobox/src/LionCombobox.js +++ b/packages/combobox/src/LionCombobox.js @@ -420,13 +420,30 @@ export class LionCombobox extends OverlayMixin(LionListbox) { } } - /* eslint-disable no-param-reassign, class-methods-use-this */ + /** + * We need to extend the repropagation prevention conditions here. + * Usually form groups with single choice will not repropagate model-value-changed of an option upwards + * if this option itself is not the checked one. We want to prevent duplicates. However, for combobox + * it is reasonable that an option can become unchecked without another one becoming checked, because + * users can enter any text they want, whether it matches an option or not. + * + * Therefore, extend the condition to fail by checking if there is any elements checked. If so, then we + * should indeed not repropagate as normally. If there is no elements checked, this will be the only + * model-value-changed event that gets received, and we should repropagate it. + * + * @param {EventTarget & import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} target + */ + _repropagationConditionFails(target) { + return super._repropagationConditionFails(target) && this.formElements?.some(el => el.checked); + } + /* eslint-disable no-param-reassign */ /** * @overridable * @param {LionOption & {__originalInnerHTML?:string}} option * @param {string} matchingString */ + // eslint-disable-next-line class-methods-use-this _onFilterMatch(option, matchingString) { const { innerHTML } = option; option.__originalInnerHTML = innerHTML; @@ -443,7 +460,7 @@ export class LionCombobox extends OverlayMixin(LionListbox) { * @param {string} [curValue] * @param {string} [prevValue] */ - // eslint-disable-next-line no-unused-vars + // eslint-disable-next-line no-unused-vars, class-methods-use-this _onFilterUnmatch(option, curValue, prevValue) { if (option.__originalInnerHTML) { option.innerHTML = option.__originalInnerHTML; @@ -451,12 +468,14 @@ export class LionCombobox extends OverlayMixin(LionListbox) { // Alternatively, an extension can add an animation here option.style.display = 'none'; } + /* eslint-enable no-param-reassign */ /** * Computes whether a user intends to autofill (inline autocomplete textbox) * @overridable * @param {{ prevValue:string, curValue:string }} config */ + // eslint-disable-next-line class-methods-use-this _computeUserIntendsAutoFill({ prevValue, curValue }) { const userIsAddingChars = prevValue.length < curValue.length; const userStartsNewWord = diff --git a/packages/combobox/test/lion-combobox.test.js b/packages/combobox/test/lion-combobox.test.js index ca065073e..6995e569f 100644 --- a/packages/combobox/test/lion-combobox.test.js +++ b/packages/combobox/test/lion-combobox.test.js @@ -4,6 +4,7 @@ import sinon from 'sinon'; import '../lion-combobox.js'; import { LionOptions } from '@lion/listbox/src/LionOptions.js'; import { browserDetection, LitElement } from '@lion/core'; +import { Required } from '@lion/form-core'; /** * @typedef {import('../src/LionCombobox.js').LionCombobox} LionCombobox @@ -409,6 +410,31 @@ describe('lion-combobox', () => { expect(o.getAttribute('aria-hidden')).to.equal('true'); }); }); + + it('works with validation', async () => { + const el = /** @type {LionCombobox} */ (await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `)); + + // open + el._comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + + mimicUserTyping(el, 'art'); + await el.updateComplete; + expect(el.checkedIndex).to.equal(0); + + mimicUserTyping(el, ''); + await el.updateComplete; + expect(el.checkedIndex).to.equal(-1); + await el.feedbackComplete; + expect(el.hasFeedbackFor).to.include('error'); + expect(el.showsFeedbackFor).to.include('error'); + }); }); }); diff --git a/packages/form-core/src/FormControlMixin.js b/packages/form-core/src/FormControlMixin.js index 03470e943..68e7af09d 100644 --- a/packages/form-core/src/FormControlMixin.js +++ b/packages/form-core/src/FormControlMixin.js @@ -794,13 +794,14 @@ const FormControlMixinImplementation = superclass => return; } - // B2. Are we a single choice choice-group? If so, halt when unchecked + // B2. Are we a single choice choice-group? If so, halt when target unchecked + // and something else is checked, meaning we will get + // another model-value-changed dispatch for the checked target // // We only send the checked changed up (not the unchecked). In this way a choice group // (radio-group, checkbox-group, select/listbox) acts as an 'endpoint' (a single Field) // just like the native