From 70585e1d2a54a5299e86194db1cb7c65021e3b65 Mon Sep 17 00:00:00 2001 From: Cory LaViska Date: Wed, 28 Dec 2022 15:31:42 -0500 Subject: [PATCH] finishing touches --- src/components/menu-item/menu-item.test.ts | 13 +- src/components/menu-item/menu-item.ts | 6 +- src/components/option/option.test.ts | 13 +- src/components/option/option.ts | 28 ++-- src/components/select/select.styles.ts | 17 ++- src/components/select/select.test.ts | 145 +++++++++++++++++---- src/components/select/select.ts | 54 ++++---- src/internal/form.ts | 2 +- 8 files changed, 193 insertions(+), 85 deletions(-) diff --git a/src/components/menu-item/menu-item.test.ts b/src/components/menu-item/menu-item.test.ts index f2c165af3..d7517b260 100644 --- a/src/components/menu-item/menu-item.test.ts +++ b/src/components/menu-item/menu-item.test.ts @@ -35,13 +35,14 @@ describe('', () => { expect(el.getTextLabel()).to.equal('Test'); }); - it('emit sl-label-change event on label change', async () => { - const el = await fixture(html` Test `); + it('emits the slotchange event when the label changes', async () => { + const el = await fixture(html` Text `); + const slotChangeHandler = sinon.spy(); - const labelChangeHandler = sinon.spy(); + el.addEventListener('slotchange', slotChangeHandler); el.textContent = 'New Text'; - el.addEventListener('sl-label-change', labelChangeHandler); - await waitUntil(() => labelChangeHandler.calledOnce); - expect(labelChangeHandler).to.have.been.calledOnce; + await waitUntil(() => slotChangeHandler.calledOnce); + + expect(slotChangeHandler).to.have.been.calledOnce; }); }); diff --git a/src/components/menu-item/menu-item.ts b/src/components/menu-item/menu-item.ts index d276e5761..ef6e32ca2 100644 --- a/src/components/menu-item/menu-item.ts +++ b/src/components/menu-item/menu-item.ts @@ -16,9 +16,6 @@ import type { CSSResultGroup } from 'lit'; * * @dependency sl-icon * - * @event sl-label-change - Emitted when the menu item's text label changes. For performance reasons, this event is only - * emitted if the default slot's `slotchange` event is triggered. It will not fire when the label is first set. - * * @slot - The menu item's label. * @slot prefix - Used to prepend an icon or similar element to the menu item. * @slot suffix - Used to append an icon or similar element to the menu item. @@ -78,9 +75,10 @@ export default class SlMenuItem extends ShoelaceElement { return; } + // When the label changes, emit a slotchange event so parent controls see it if (textLabel !== this.cachedTextLabel) { this.cachedTextLabel = textLabel; - this.emit('sl-label-change'); + this.emit('slotchange', { bubbles: true, composed: false, cancelable: false }); } } diff --git a/src/components/option/option.test.ts b/src/components/option/option.test.ts index e3898a980..32029676b 100644 --- a/src/components/option/option.test.ts +++ b/src/components/option/option.test.ts @@ -31,13 +31,14 @@ describe('', () => { expect(el.getAttribute('aria-disabled')).to.equal('true'); }); - it('emit sl-label-change when the label changes', async () => { - const el = await fixture(html` Test `); + it('emits the slotchange event when the label changes', async () => { + const el = await fixture(html` Text `); + const slotChangeHandler = sinon.spy(); - const labelChangeHandler = sinon.spy(); + el.addEventListener('slotchange', slotChangeHandler); el.textContent = 'New Text'; - el.addEventListener('sl-label-change', labelChangeHandler); - await waitUntil(() => labelChangeHandler.calledOnce); - expect(labelChangeHandler).to.have.been.calledOnce; + await waitUntil(() => slotChangeHandler.calledOnce); + + expect(slotChangeHandler).to.have.been.calledOnce; }); }); diff --git a/src/components/option/option.ts b/src/components/option/option.ts index b13298cac..a15994804 100644 --- a/src/components/option/option.ts +++ b/src/components/option/option.ts @@ -2,7 +2,6 @@ import { html } from 'lit'; import { customElement, property, query, state } from 'lit/decorators.js'; import { classMap } from 'lit/directives/class-map.js'; import ShoelaceElement from '../../internal/shoelace-element'; -import { getTextContent } from '../../internal/slot'; import { watch } from '../../internal/watch'; import { LocalizeController } from '../../utilities/localize'; import '../icon/icon'; @@ -17,10 +16,6 @@ import type { CSSResultGroup } from 'lit'; * * @dependency sl-icon * - * @event sl-label-change - Emitted when the option's label changes. For performance reasons, this event is only emitted - * when the default slot's `slotchange` event is triggered. It will not fire when the label is first set. Useful for - * parent controls that want to observe label changes without attaching an expensive mutation observer. - * * @slot - The option's label. * @slot prefix - Used to prepend an icon or similar element to the menu item. * @slot suffix - Used to append an icon or similar element to the menu item. @@ -44,8 +39,12 @@ export default class SlOption extends ShoelaceElement { @query('.option__label') defaultSlot: HTMLSlotElement; - /** The option's value. When selected, the containing form control will receive this value. */ - @property() value = ''; + /** + * The option's value. When selected, the containing form control will receive this value. The value must be unique + * from other options in the same group. Values may not contain spaces, as spaces are used as delimiters when listing + * multiple values. + */ + @property({ reflect: true }) value = ''; /** Draws the option in a disabled state, preventing selection. */ @property({ type: Boolean, reflect: true }) disabled = false; @@ -58,7 +57,7 @@ export default class SlOption extends ShoelaceElement { /** Returns a plain text label based on the option's content. */ getTextLabel() { - return this.textContent ?? ''; + return (this.textContent ?? '').trim(); } @watch('disabled') @@ -71,8 +70,16 @@ export default class SlOption extends ShoelaceElement { this.setAttribute('aria-selected', this.selected ? 'true' : 'false'); } + @watch('value') + handleValueChange() { + if (this.value.includes(' ')) { + console.error(`Option values cannot include a space. All spaces have been replaced with underscores.`, this); + this.value = this.value.replace(/ /g, '_'); + } + } + handleDefaultSlotChange() { - const textLabel = getTextContent(this.defaultSlot); + const textLabel = this.getTextLabel(); // Ignore the first time the label is set if (typeof this.cachedTextLabel === 'undefined') { @@ -80,9 +87,10 @@ export default class SlOption extends ShoelaceElement { return; } + // When the label changes, emit a slotchange event so parent controls see it if (textLabel !== this.cachedTextLabel) { this.cachedTextLabel = textLabel; - this.emit('sl-label-change'); + this.emit('slotchange', { bubbles: true, composed: false, cancelable: false }); } } diff --git a/src/components/select/select.styles.ts b/src/components/select/select.styles.ts index 8f86563d2..f9c896ea6 100644 --- a/src/components/select/select.styles.ts +++ b/src/components/select/select.styles.ts @@ -155,7 +155,8 @@ export default css` border-radius: var(--sl-input-border-radius-small); font-size: var(--sl-input-font-size-small); min-height: var(--sl-input-height-small); - padding: 0 var(--sl-input-spacing-small); + padding-block: 0; + padding-inline: var(--sl-input-spacing-small); } .select--small .select__clear { @@ -167,8 +168,8 @@ export default css` } .select--small.select--multiple .select__combobox { - padding-inline-start: 0; padding-block: 2px; + padding-inline-start: 0; } .select--small .select__tags { @@ -179,7 +180,8 @@ export default css` border-radius: var(--sl-input-border-radius-medium); font-size: var(--sl-input-font-size-medium); min-height: var(--sl-input-height-medium); - padding: 0 var(--sl-input-spacing-medium); + padding-block: 0; + padding-inline: var(--sl-input-spacing-medium); } .select--medium .select__clear { @@ -203,7 +205,8 @@ export default css` border-radius: var(--sl-input-border-radius-large); font-size: var(--sl-input-font-size-large); min-height: var(--sl-input-height-large); - padding: 0 var(--sl-input-spacing-large); + padding-block: 0; + padding-inline: var(--sl-input-spacing-large); } .select--large .select__clear { @@ -291,7 +294,8 @@ export default css` background: var(--sl-panel-background-color); border: solid var(--sl-panel-border-width) var(--sl-panel-border-color); border-radius: var(--sl-border-radius-medium); - padding: var(--sl-spacing-x-small) 0; + padding-block: var(--sl-spacing-x-small); + padding-inline: 0; overflow: auto; overscroll-behavior: none; @@ -309,6 +313,7 @@ export default css` font-size: var(--sl-font-size-small); font-weight: var(--sl-font-weight-semibold); color: var(--sl-color-neutral-500); - padding: var(--sl-spacing-x-small) var(--sl-spacing-x-large); + padding-block: var(--sl-spacing-x-small); + padding-inline: var(--sl-spacing-x-large); } `; diff --git a/src/components/select/select.test.ts b/src/components/select/select.test.ts index 3a1ae1063..18cef8e1f 100644 --- a/src/components/select/select.test.ts +++ b/src/components/select/select.test.ts @@ -3,6 +3,7 @@ import { sendKeys } from '@web/test-runner-commands'; import sinon from 'sinon'; import { waitForEvent } from '../../internal/event'; import { clickOnElement } from '../../internal/test'; +import { serialize } from '../../utilities/form'; import type SlOption from '../option/option'; import type SlSelect from './select'; @@ -172,40 +173,108 @@ describe('', () => { expect(displayInput.getAttribute('aria-expanded')).to.equal('false'); }); - it('should focus on the displayInput when constraint validation occurs', async () => { - const el = await fixture(html` -
- - Option 1 - Option 2 - Option 3 - -
- `); - const select = el.querySelector('sl-select')!; - el.requestSubmit(); + describe('when using constraint validation', () => { + it('should be valid by default', async () => { + const el = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const select = el.querySelector('sl-select')!; + expect(select.checkValidity()).to.be.true; + }); - expect(select.shadowRoot!.activeElement).to.equal(select.displayInput); + it('should be invalid when required and empty', async () => { + const el = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const select = el.querySelector('sl-select')!; + expect(select.checkValidity()).to.be.false; + }); + + it('should focus on the displayInput when constraint validation occurs', async () => { + const el = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const select = el.querySelector('sl-select')!; + el.requestSubmit(); + expect(select.shadowRoot!.activeElement).to.equal(select.displayInput); + }); }); - it('should update the display label when an option changes', async () => { - const el = await fixture(html` - - Option 1 - Option 2 - Option 3 - - `); - const displayInput = el.shadowRoot!.querySelector('.select__display-input')!; - const option = el.querySelector('sl-option')!; + describe('when serializing', () => { + it('should serialize its name and value with FormData', async () => { + const form = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const formData = new FormData(form); + expect(formData.get('a')).to.equal('option-1'); + }); - expect(displayInput.value).to.equal('Option 1'); + it('should serialize its name and value in FormData when multiple options are selected', async () => { + const form = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const formData = new FormData(form); + expect(formData.getAll('a')).to.include('option-2'); + expect(formData.getAll('a')).to.include('option-3'); + }); - option.textContent = 'updated'; - await oneEvent(option, 'sl-label-change'); - await el.updateComplete; + it('should serialize its name and value in JSON', async () => { + const form = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const json = serialize(form); + expect(json.a).to.equal('option-1'); + }); - expect(displayInput.value).to.equal('updated'); + it('should serialize its name and value in JSON when multiple options are selected', async () => { + const form = await fixture(html` +
+ + Option 1 + Option 2 + Option 3 + +
+ `); + const json = serialize(form); + expect(JSON.stringify(json)).to.equal(JSON.stringify({ a: ['option-2', 'option-3'] })); + }); }); describe('when resetting a form', () => { @@ -237,4 +306,24 @@ describe('', () => { expect(select.value).to.equal('option-1'); }); }); + + it('should update the display label when an option changes', async () => { + const el = await fixture(html` + + Option 1 + Option 2 + Option 3 + + `); + const displayInput = el.shadowRoot!.querySelector('.select__display-input')!; + const option = el.querySelector('sl-option')!; + + expect(displayInput.value).to.equal('Option 1'); + + option.textContent = 'updated'; + await oneEvent(option, 'slotchange'); + await el.updateComplete; + + expect(displayInput.value).to.equal('updated'); + }); }); diff --git a/src/components/select/select.ts b/src/components/select/select.ts index a36a87be3..cad0ce06e 100644 --- a/src/components/select/select.ts +++ b/src/components/select/select.ts @@ -85,10 +85,16 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon @property() name = ''; /** - * The current value of the select, submitted as a name/value pair with form data. If `multiple` is enabled, this - * property will be an array. Otherwise, it will be a string. + * The current value of the select, submitted as a name/value pair with form data. When `multiple` is enabled, the + * value will be a space-delimited list of values based on the options selected. */ - @property() value: string | string[] = ''; + @property({ + converter: { + fromAttribute: (value: string) => value.split(' '), + toAttribute: (value: string[]) => value.join(' ') + } + }) + value: string | string[] = ''; /** The default value of the form control. Primarily used for resetting the form control. */ @defaultValue() defaultValue: string | string[] = ''; @@ -157,10 +163,6 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon this.open = false; } - firstUpdated() { - this.invalid = !this.checkValidity(); - } - /** Checks for validity but does not show the browser's validation message. */ checkValidity() { return this.valueInput.checkValidity(); @@ -424,18 +426,22 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon private handleDefaultSlotChange() { const allOptions = this.getAllOptions(); + const value = Array.isArray(this.value) ? this.value : [this.value]; const values: string[] = []; // Check for duplicate values in menu items allOptions.forEach(option => { if (values.includes(option.value)) { - console.error(`A duplicate value has been found in . All options must have unique values.`, option); + console.error( + `An option with duplicate values has been found in . All options must be unique.`, + option + ); } values.push(option.value); }); - // Update the selection since it probably changed - this.selectionChanged(); + // Select only the options that match the new value + this.setSelectedOptions(allOptions.filter(el => value.includes(el.value))); } // Gets an array of all elements @@ -500,11 +506,10 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon this.selectionChanged(); } - // This method must be called whenever the selection changes. It will sync the selected options cache, update the - // current value, and update the display value. + // This method must be called whenever the selection changes. It will update the selected options cache, the current + // value, and the display value private selectionChanged() { - console.log('selectionChanged'); - // Update selection options cache + // Update selected options cache this.selectedOptions = this.getAllOptions().filter(el => el.selected); // Update the value and display label @@ -517,7 +522,16 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon } // Update validity - this.invalid = !this.checkValidity(); + this.updateComplete.then(() => (this.invalid = !this.checkValidity())); + } + + @watch('disabled', { waitUntilFirstUpdate: true }) + handleDisabledChange() { + // Close the listbox when the control is disabled + if (this.disabled) { + this.open = false; + this.handleOpenChange(); + } } @watch('value', { waitUntilFirstUpdate: true }) @@ -553,12 +567,7 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon @watch('open', { waitUntilFirstUpdate: true }) async handleOpenChange() { - if (this.disabled) { - this.hide(); - return; - } - - if (this.open) { + if (this.open && !this.disabled) { // Reset the current option this.setCurrentOption(this.selectedOptions[0] || this.getFirstOption()); @@ -628,8 +637,6 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon ${this.label} - / Value: ${Array.isArray(this.value) ? this.value.join(' + ') : this.value} -
diff --git a/src/internal/form.ts b/src/internal/form.ts index a1ea09c37..e79c7fac0 100644 --- a/src/internal/form.ts +++ b/src/internal/form.ts @@ -151,7 +151,7 @@ export class FormSubmitController implements ReactiveController { // injecting the name/value on a temporary button, so we can just skip them here. const isButton = this.host.tagName.toLowerCase() === 'sl-button'; - if (!disabled && !isButton && typeof name === 'string' && typeof value !== 'undefined') { + if (!disabled && !isButton && typeof name === 'string' && name.length > 0 && typeof value !== 'undefined') { if (Array.isArray(value)) { (value as unknown[]).forEach(val => { event.formData.append(name, (val as string | number | boolean).toString());