-
-
Notifications
You must be signed in to change notification settings - Fork 262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(multi-select): render checkboxes for form data #1835
Conversation
The MultiSelect wasn't rendering form-submittable inputs. This fix adds two new props to help customize how the hidden inputs render. See #1742
src/MultiSelect/MultiSelect.svelte
Outdated
* Override the item name, title, labelText passed to the checkbox input | ||
* @type {(item: MultiSelectItem) => { name?: string; labelText?: any; title?: string; }} | ||
* Override the item name, title, labelText, or value passed to the user-selectable checkbox input as well as the hidden inputs. | ||
* @type {(item: MultiSelectItem) => { name?: string; labelText?: any; title?: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/MultiSelect/MultiSelect.svelte
Outdated
* Set to `true` to only render selected options as hidden inputs for form submission. | ||
* @type {boolean} | ||
*/ | ||
export let selectedOnly = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you run Prettier to format this file? I think that the trailing semicolons will be applied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM – elegant fix and update to the docs.
Thanks @metonym, though I think I had a new realization: if I conditionally hide the ListBox using Reference: carbon-components-svelte/src/MultiSelect/MultiSelect.svelte Lines 501 to 506 in 276465a
Refactor: - {#if open}
+ <div class:hidden="{!open}">
<ListBoxMenu
aria-label="{ariaLabel}"
id="{id}"
aria-multiselectable="true"
>
<!-- stuff -->
+<style>
+ .hidden {
+ display: none;
+ }
+</style> Minimal demo: https://www.sveltelab.dev/ibxj8ki83lee147 This is great for progressive enhancement down the road, once the popover API becomes stable. With that said, should I refactor to use the above logic? I can probably remove the new props as well and have my own project make use of the checkboxes; leveraging What do you think? |
That approach seems valid. I'll admit that for one-line style changes, I have a slight preference for using the |
Display ListBox via CSS
Fixes #1742
MultiSelect up til now wasn't rendering hidden inputs in order to be submittable within a
<form>
. With this enhancement:MultiSelect renders all items as<input type="hidden" name="some-id" value="true" />
usingvalue="true"
for checked items andvalue="false"
for unchecked items by default.New propselectedOnly
only renders hidden inputs for user-selected items.New propcombineValues
renders a single hidden input with comma-separated values, or using a consumer-provided delimiter.itemToInput
is enhanced to perform double duty for user-visible checkboxes as well as the hidden inputs, mainly to help overridename
andvalue
attributes among the hidden inputs.Feature (3) was a personal need in order to submit an array of values within a form, but I felt the default behaviour should be checkbox-like as per point (1) assuming that's what consumers of MultiSelect would expect.See included documentation for more examples.