Multi Select #2
Multi Select #2
Conversation
…select into multi-select
…select into multi-select
| @@ -42,7 +42,7 @@ | |||
| "@types/jest": "^29.5.7", | |||
| "@vitejs/plugin-vue": "^4.2.3", | |||
| "@vue/test-utils": "^2.4.1", | |||
| "happy-dom": "^12.10.3", | |||
| "jsdom": "^22.1.0", | |||
There was a problem hiding this comment.
had to change this because happy dom has a bug which doesn't let you look at css properties (which are required for functions like isVisible())
There was a problem hiding this comment.
Works great!
It took me a while to remember exactly what the existing behaviour is - it's a bit eccentric but I guess people are used to it! So if all the children of a parent are selected then they don't show as being individually selected at the top, but instead the parent item is selected until you unselect something underneath it. So a parent item being selected implies that all its descendents are selected. And this component does appear to exactly replicate that!
I think the existing appearance in the badges at the top, with the "| x", is quite good for indicating to the user that these can be removed by clicking, so I'd be for including that.
getCheckedObject is a bit confusing at first, but efficient to do that in a single pass rather than checking paths of all the items all over the place, so i agree that's a good way to go.
| @@ -0,0 +1,157 @@ | |||
| <template> | |||
There was a problem hiding this comment.
This component is so similar to SingleSelect, that it feels like it could just be a single component which operates in two different modes, or could at least share common code via a mixin/composable and a common child component for the shared template - but i think the former might be simpler, especially if the complex handleSelect logic gets moved to a util.
There was a problem hiding this comment.
i completely agree, i actually made a start on doing that before I saw the comment, there are a few quirks of each like we control the dropdown menu showing only for the single select but it is very small, i have had a go at making something simpler, will make a PR for that!
| @@ -71,3 +71,135 @@ export const collapseOptions = (array: FlatOption[], optionPath: string[]) => { | |||
| index++; | |||
| }; | |||
| }; | |||
|
|
|||
| export const getNestedChildrenIds = (flatOptions: FlatOption[], ids: string[]) => { | |||
There was a problem hiding this comment.
| export const getNestedChildrenIds = (flatOptions: FlatOption[], ids: string[]) => { | |
| // For a full array of flattened options, and an array of selected ids, return an array containing those ids as | |
| // well as the ids of all their descendents | |
| export const getDescendentIds = (flatOptions: FlatOption[], ids: string[]) => { |
| } | ||
| } | ||
| recordChildStatus(parentId, childCheckedRecord, status); | ||
| checkedObject[op.path.join("/")] = status; |
There was a problem hiding this comment.
Why use joined path here rather than id?
| } | ||
| }; | ||
|
|
||
| export const getCheckedObject = (flatOptions: FlatOption[], checkedValues: string[]): CheckObject => { |
There was a problem hiding this comment.
| export const getCheckedObject = (flatOptions: FlatOption[], checkedValues: string[]): CheckObject => { | |
| // For a full array of flattened options, and an array of all selected ids, return an object indicating the checked | |
| // value to display in the multi-select for each option | |
| export const getCheckedObject = (flatOptions: FlatOption[], selectedIds: string[]): CheckObject => { |
| 2. Any parents (found from the ids in its path) are unchecked too. This is because | ||
| one of their children was unchecked so they cannot be fully checked anymore | ||
| */ | ||
| emit("update:modelValue", [...props.modelValue?.filter(val => !uncheckIds.includes(val)) || []]); |
There was a problem hiding this comment.
Don't think you need the spread operator here?
| 2. Any parents (found from the ids in its path) are unchecked too. This is because | ||
| one of their children was unchecked so they cannot be fully checked anymore | ||
| */ | ||
| emit("update:modelValue", [...props.modelValue?.filter(val => !uncheckIds.includes(val)) || []]); |
There was a problem hiding this comment.
Could we just work out the updated modelValue in all these conditional arms, and do a single emit at the end of the method?
Might be worth pulling the logic out into a util method to get the updated selection, then this handler can just call that then do the emit.
| const parentIds = op.path; | ||
|
|
||
| // we loop from the highest parent's id in this option's path | ||
| // to the lowest which is itself's id in its path and break |
There was a problem hiding this comment.
| // to the lowest which is itself's id in its path and break | |
| // to the lowest, which is its own id, and break |
| }; | ||
|
|
||
| export const getTopLevelOptionTags = ( | ||
| selectedTags: FlatOption[], |
There was a problem hiding this comment.
| selectedTags: FlatOption[], | |
| selectedOptions: FlatOption[], |
Description
Once again about 700 lines are just
package-lockchanges! This PR adds the nested multi select into this package. Please do try and change the options inApp.vueto mess around with different cases. I think after this PR, we are good to add this into Hint (after version change too I guess). I will be working on the search functionality too (I have already started on this) however I think after these basic features are in, we can use it in our Table Tab PRs to get the ball rolling on that.To run:
To test: