feat(#1233): implement a MultiselectDropdown component by combining popover and checkboxlist#3936
feat(#1233): implement a MultiselectDropdown component by combining popover and checkboxlist#3936willcodeforcoffee wants to merge 6 commits into
Conversation
|
Preview links
Built from commit a384405. Previews are removed automatically when this PR closes. |
022f377 to
2d4037b
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new DropdownMultiselect UI component to the web-components library (Svelte custom element) and exposes it through the React and Angular wrapper libraries, with accompanying unit/browser tests and PR demo routes.
Changes:
- Added
goa-dropdown-multiselectweb component that composesgoa-popover+goa-checkbox-listand supports filtering and truncate-label behavior. - Added React + Angular wrapper components and exports, plus common types for size and change-event detail.
- Added test coverage (Svelte unit tests, React unit + browser tests, Angular unit tests) and demo routes in the PR apps.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/web-components/src/index.ts | Exports the new DropdownMultiselect web component. |
| libs/web-components/src/components/dropdown-multiselect/DropdownMultiselect.svelte | Implements the new goa-dropdown-multiselect custom element (popover + checkbox list). |
| libs/web-components/src/components/dropdown-multiselect/DropdownMultiselect.spec.ts | Adds Svelte unit tests for rendering, ARIA, keyboard interaction, and change events. |
| libs/web-components/src/components/checkbox-list/CheckboxList.svelte | Adjusts checkbox-list change handling (and currently adds debug logging). |
| libs/react-components/src/lib/dropdown-multiselect/dropdown-multiselect.tsx | Adds the React wrapper component for goa-dropdown-multiselect. |
| libs/react-components/src/lib/dropdown-multiselect/dropdown-multiselect.spec.tsx | Adds React unit tests for wrapper prop wiring and _change handling. |
| libs/react-components/src/index.ts | Re-exports the new React wrapper. |
| libs/react-components/specs/dropdown-multiselect.browser.spec.tsx | Adds browser-level interaction tests for the React wrapper + web component behavior. |
| libs/common/src/lib/common.ts | Adds shared types (GoabDropdownMultiselectSize, GoabDropdownMultiselectOnChangeDetail). |
| libs/angular-components/src/lib/components/index.ts | Re-exports the new Angular wrapper. |
| libs/angular-components/src/lib/components/dropdown-multiselect/dropdown-multiselect.ts | Adds the Angular wrapper component (CVA) for goa-dropdown-multiselect. |
| libs/angular-components/src/lib/components/dropdown-multiselect/dropdown-multiselect.spec.ts | Adds Angular unit tests for wrapper bindings and emitted change events. |
| apps/prs/react/src/routes/features/feat1233.tsx | Adds a React demo route for the new component (currently includes debug logging). |
| apps/prs/react/src/app/routes/features/feat1233.route.ts | Registers the React demo route. |
| apps/prs/angular/src/routes/features/feat1233/feat1233.route.json | Registers the Angular demo route metadata. |
| apps/prs/angular/src/routes/features/feat1233/feat1233.component.ts | Adds Angular demo component logic for controlled selections. |
| apps/prs/angular/src/routes/features/feat1233/feat1233.component.html | Adds Angular demo template showcasing variants (filterable, truncateLabel, etc.). |
|
I've updated the styling and added references to the new component design tokens and fallbacks. The desk token PR is GovAlta/design-tokens#161. |
bdfranck
left a comment
There was a problem hiding this comment.
I looked at the latest changes...
- ✅ I can open the dropdown by keyboard
- ✅ I can can navigate the items by arrow keys or
tab - ✅ I can see the label overflow properly
- ✅ I can see the label truncate properly
- ✅ I can see a disabled state
- ✅ I can see an error state
- ✅ I can filter options
Looking great! I added a commit with two small tweaks. There's only two minor things so far:
- When there are no results, can we add "No matches found" text with
secondarycolor? - Is there an easy way to prevent the
cursor: pointeron the disabled state? It suggests that the item is still interactive.
8042331 to
1b113d5
Compare
1b113d5 to
f814b3f
Compare
| @@ -0,0 +1,606 @@ | |||
| <svelte:options | |||
There was a problem hiding this comment.
Testing feedback:
- Home/End: (Optional) https://www.w3.org/WAI/ARIA/apg/patterns/listbox/#keyboardinteraction Can we make Home to move focus to the first option and End to move focus to the last option, W3 recommends for list >= 5 items and I think it makes sense.
- When we focus on the multi select, filterable, we should be able to see the cursor appears right away and we can type the keywords. Right now, we need to tab to focus, press Enter before we can type the search string
- Search logic: On our current Dropdown if type "Deer" or "deer" it will display "Red Deer" but on the Multi select component it displays nothing matched.
- When I type "Calgary" -> arrow down -> press Enter -> I cannot use keyboard only to move up to the text field and continue to search (Shift + Tabs) doesn't work. We can refer to https://mantine.dev/core/multi-select/ this, Searchable example, where we search "Angular", arrow down, Enter to select, the text field clears the keyword "Angular", we can continue to type another one to continue without using a mouse.
- I add the multi select inside the modal, I use a keyboard and press, select an option and then I press ESC to escape the multi select, however it closes the modal I am in. Expected to exit the multi select only, not the modal.
- Filterable textfield should have a leading icon "search" similar to Dropdown.
- If I set Filerable true and selectAll true, and then I test: "Apple" and press Apple to choose it, then I click "Select All", the behaviour is very weird. We expect the search textfield cleared or update the label to reflect the choice "Select All", otherwise it behaves "conflicted"
select-all-filterable-multi-select.mov
| GoabDropdownItem, | ||
| GoabDropdownMultiselect, | ||
| GoabFormItem, | ||
| GoabText, |
There was a problem hiding this comment.
Unused import GoabText
| }, | ||
| ], | ||
| webComponents: `<goa-form-item version="2" label="Select fruits" mb="l"> | ||
| <goa-dropdown-multiselect version="2" name="fruits" width="320px"> |
There was a problem hiding this comment.
do we have version attribute for this new component?
| <GoabDropdownItem value="pear" label="Pear" /> | ||
| </GoabDropdownMultiselect> | ||
| </GoabFormItem>`, | ||
| angular: `<goa-form-item label="Select fruits" mb="l"> |
There was a problem hiding this comment.
should be goab-form-item and goab-dropdown-multiselect
| version="2" | ||
| name="fruits" | ||
| width="320px" | ||
| truncateLabel="true" |
There was a problem hiding this comment.
should be truncate-label right?
| customElement={{ | ||
| tag: "goa-dropdown-multiselect", | ||
| props: { | ||
| testid: { type: "String", attribute: "testid", reflect: true }, |
There was a problem hiding this comment.
we do not need to declare tesid here, unless we want to make it like tesId(camelCase) or test-id in the attribute. If we keep using tesid then we should remove this line. To make it similar to other components, I think we should keep tesid and remove this line
| tag: "goa-dropdown-multiselect", | ||
| props: { | ||
| testid: { type: "String", attribute: "testid", reflect: true }, | ||
| truncateLabel: { |
There was a problem hiding this comment.
Can we change this truncateLabel to labelDisplay and instead of boolean, can we make it as list | count, the reason why I suggest so is if in the future we want to add more way to display this label (chips?) then this property will open more choices for us
| attribute: "truncate-label", | ||
| reflect: true, | ||
| }, | ||
| selectAll: { type: "String", attribute: "select-all", reflect: true }, |
There was a problem hiding this comment.
Can we rename this to be allowSelectAll or withSelectAll, selectAll starts with a verb so the type boolean makes me confused. Also can we follow Drawer.svelte with open property, so we can make type here as Boolean instead of string? Then we do not need toBoolean anymore
| /** Text shown when nothing is selected. */ | ||
| export let placeholder: string = ""; | ||
| /** Enables filtering of options by typing in the trigger. */ | ||
| export let filterable: string = "false"; |
There was a problem hiding this comment.
We can declare filterable on the top as a type Boolean like open inside Drawer.svelte
| /** Provides an accessible label when no visible label is associated. */ | ||
| export let arialabel: string = ""; | ||
| /** References an external element that labels this component. */ | ||
| export let arialabelledby: string = ""; |
There was a problem hiding this comment.
Like recently developed component MenuButton, I think we should declare arialabelledby as ariaLabelledBy on the options tags above and make attribute as aria-labelledby, also it would match the ARIA name
| /** Sets a data-testid attribute for automated testing. */ | ||
| export let testid: string = ""; | ||
| /** Provides an accessible label when no visible label is associated. */ | ||
| export let arialabel: string = ""; |
There was a problem hiding this comment.
rename as ariaLabel and define on options tag above, named attribute as aria-label
| /** References an external element that labels this component. */ | ||
| export let arialabelledby: string = ""; | ||
| /** Sets the maximum height of the dropdown content area. */ | ||
| export let maxheight: string = "276px"; |
There was a problem hiding this comment.
same here, make it maxHeight and attribute name max-height
| } | ||
|
|
||
| function onChildDestroyed(detail: DropdownItemDestroyRelayDetail) { | ||
| _options = _options.filter((o) => o.value !== detail.value); |
There was a problem hiding this comment.
When this happen, can we reset the value, or at least keep track value to make sure it synced with available options, and also dispatch a _change event if it happens too
| case " ": | ||
| if (!_isOpen) { | ||
| e.preventDefault(); | ||
| focusFirstCheckbox(); |
There was a problem hiding this comment.
We dont need to call focusFirstCheckbox here, reason is: _isOpen we pass to goa-popover below, will be handled by Popover.svelte with function syncPopoverOpenState, and onMount above we already listen to _open from goa-popover and trigger this focusFirsCheckbox again, which means the current way will go through focusFirstCheckbox 2 times, with requestAnimation delay there
| : listItems; | ||
| } | ||
|
|
||
| function focusFirstCheckbox() { |
There was a problem hiding this comment.
focusFirstCheckbox, focusNextCheckboxandfocusPreviousCheckboxhave duplicated logic, we should merge 3 of them intofocusCheckboxAt(index)for example, also, inside the function, we can make use offindFirstFocusableNode([checkboxes[_focusedIndex]])?.focus()fromutils` instead of querySelector with shadowRoot
| /** Sets a fixed width for the component and popover panel. */ | ||
| export let width: string = ""; | ||
| /** Sets the size variant. */ | ||
| export let size: "default" | "compact" = "default"; |
There was a problem hiding this comment.
for size, we should add typeValidator like other components and trigger at onMount
|
|
||
| <div | ||
| id={_contentId} | ||
| role="none" |
There was a problem hiding this comment.
perhaps we should make role=\"dialog\" at least, instead of none and we may add aria-label=placeholder | \"Options\" for example, otherwise the aria-controls at line 363 will point to this and screen reader will not be able to announce the relationship between the trigger above and this div
|
|
||
| .select-all-divider { | ||
| border: none; | ||
| border-top: var(--goa-border-width-s) solid var(--goa-color-greyscale-200); |
There was a problem hiding this comment.
Is there a reason why this border-top does not have a token like the rest?
| id={_contentId} | ||
| role="none" | ||
| class="content" | ||
| style="max-height: {maxheight}; overflow-y: auto;" |
There was a problem hiding this comment.
do we need inline style overflow-y here, I see we define under css .content line 594 too
| } from "../dropdown/DropdownItem.svelte"; | ||
|
|
||
| /** @required Identifier for the group. Used in change events. */ | ||
| export let name: string; |
There was a problem hiding this comment.
consider to use validateRequired for this name property, under onMount
|
|
||
| function handleFilterKeydown(e: KeyboardEvent) { | ||
| if (e.key === "ArrowDown") { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
Missing e.stopPropagation(), consequence is when we search New and we have 2 choices New Brunswick and Newfoundland.., we press ArrowDown, it instead of jumping to the first choice New Brunswick, it jumps to the second choice
| $: isError = toBoolean(error); | ||
| $: isFilterable = toBoolean(filterable); | ||
| $: isSelectAll = toBoolean(selectAll); | ||
| $: if (!Array.isArray(value)) value = []; |
There was a problem hiding this comment.
consider to do Array.isArray(value) ? value : [], reason is Angular/React also pass down the value, if we override it value internal with [], it can become a loop of reactivity from React/Angular to our internal value hook here
| "name": "maxHeight", | ||
| "type": "string", | ||
| "required": false, | ||
| "default": "276px", |
There was a problem hiding this comment.
I run npm run build:prod and this value becomes null
This PR creates a dropdown multiselect list component by combining the CheckboxList and Popover Components.
The only AC left in Story 1233 is a link to a Figma that doesn't exist anymore. 🤷
The Multiselect has documentation as part of the PR https://govalta.github.io/ui-components/pr-preview/pr-3936/components/dropdown-multiselect/#angular#properties
We've added these features:
truncateLabel)This closes #1233.
Make sure that you've checked the boxes below before you submit the PR
Steps needed to test
This is a brand new component. Testing pages are available in the React and Angular playground, as well as documentation pages. It would be great testing for you to add it to your pages as well.