Single Select #1

M-Kusumgar · 2023-11-02T17:36:17Z

Description

Don't be afraid! Most of the diff lines are package-lock.json and boilerplate. This PR adds the nested single select into this package. I have not updated the README, incorporated CodeCov or added a test workflow yet, these will happen once the multiselect is added but to see demo run:

npm i
npm run dev

Feel free to change the data in the App.vue file to test out the different cases.

To test run:

npm run test

This project uses vite.

Guide

My general approach was to use an existing dropdown component and modify it so we can have nested options. The way this works is we first flattenOptions which will convert

[{
  id: "id1",
  label: "label1",
  children: {
    id: "id1_1",
    label: "label1_1"
  }
}]

into

[
  {
    id: "id1",
    label: "label1",
    hasChildren: true,
    open: false,
    show: true,
    path: "/id1"
  },
  {
    id: "id1_1",
    label: "label1_1",
    hasChildren: false,
    show: false,
    path: "/id1/id1_1"
  }
]

The path keeps track of the directory like structure so we can find if something is a child of another by comparing strings. The show will control what options are shown in the menu so when we expand a collapsible item, all the shows of the children will flip to true. The collapse will scan for any path with the same prefix as the item being collapsed and collapse those.

r-ash

Thanks for talking me through my dumb questions on this. This looks really great to me and I like doing the recursion step in your utils so it is really easy to test. Might be worth changing your path to an array to avoid having to do the string splitting which would fail is someone has a / in their ID. But otherwise this looks really solid to me

EmmaLRussell

Neat! Works really well - couple of suggestions but up to you.

Are you going to add a gha to run the tests and coverage?

EmmaLRussell · 2023-11-08T16:15:28Z

package.json

@@ -0,0 +1,44 @@
+{
+  "name": "vue-nested-multiselect",


We've been prefacing our npm packages with @reside-ic, and including repository, author, license, homepage so they show up nicely on npm.

EmmaLRussell · 2023-11-08T16:21:06Z

src/App.vue

@@ -0,0 +1,66 @@
+<template>


Feels like this doesn't really belong in src if it's just for manual testing, and same for main.ts. Could they be pulled out into folder called something like demo?

EmmaLRussell · 2023-11-08T16:26:56Z

src/components/DropdownItem.vue

+                <vue-feather type="chevron-right"
+                            v-show="!option.open"
+                            class="icon"/>
+                <vue-feather type="chevron-down"
+                            v-show="option.open"
+                            class="icon"/>


This would be a bit more compact, but not too fussed

Suggested change

<vue-feather type="chevron-right"

v-show="!option.open"

class="icon"/>

<vue-feather type="chevron-down"

v-show="option.open"

class="icon"/>

<vue-feather :type="option.open ? 'chevron-down' : 'chevron-right'"

class="icon"/>

so the reason i did this was because v-show will actually load both of them so if someone were to spam click, this performs slightly better, obviously not noticeably but thats why i tried to do it this way, happy to change it though! up to you really XD

Ah ok - leave it as it is if it behaves more nicely this way!

EmmaLRussell · 2023-11-08T16:32:08Z

src/components/utils.ts

@@ -0,0 +1,65 @@
+import { FlatOption, Option } from "./types";


Should types.ts and utils.ts live outside components since they aren't... ?

yep can move them! done

EmmaLRussell · 2023-11-08T16:38:19Z

src/components/DropdownItem.vue

+            expand: {
+                type: Function as PropType<(payload: MouseEvent) => void>,
+                default: () => null
+            },
+            collapse: {
+                type: Function as PropType<(payload: MouseEvent) => void>,
+                default: () => null
+            },


Not sure why these are props? These are emitted events aren't they?

haha i noticed this mistake when wrapping up the multiselect but was just gonna change it in the multiselect branch if no one noticed 😆 , done. This was from a previous commit where i was passing closures in as props and calling them, no idea why but decided to use emits now

EmmaLRussell · 2023-11-08T17:12:12Z

src/components/utils.ts

+    return JSON.stringify(array1) === JSON.stringify(array2);
+};
+
+export const expandOptions = (array: FlatOption[], optionPath: string[]): FlatOption[] => {


I do wonder if it would be a bit more Vue-y to modify the array in place rather than map to a whole new array. I "think" that should work if the FlatOption array was created as reactive when initialised.

Or maybe ref would be fine - can never remember how it treats arrays, but I'm sure it can handle changing arrays!

ref was indeed fine XD

EmmaLRussell · 2023-11-08T17:33:19Z

src/components/utils.ts

+        }
+
+        // if you are a child of a previously opened option then you should show
+        const show = openOptionsPaths.some(openOpPath => {


I don't really understand why previously opened paths are relevant here. Shouldn't it just be modifying show to true for anything immediately under the selected path, and leaving everything else alone?
And would it make sense to take advantage of the fact that we know the options are in order, so that if we find the next sibling (or higher?) of the selected path we can stop changing things (and we don't need to change anything before the selected path)?
So, could you have something like this (for modifying in place)?

const expandOptions = (array: FlatOption[], optionPath: stirng[]) => { let index = array.findIndex((fo: FlatOption) => arraysAreEqual(fo.path, optionPath)) + 1; // loop until end of all options, or next sibling (or higher level option) while (index < array.length && array[index].path.length > optionPath.length ) { // only change immediate children if (array[index].path.length === optionPath.length + 1) { array[index].show = true; } index = index + 1; } }

Alternatively while loop could check that the path starts with the optionPath path (rather than just looking at path length).

i really like this idea of just mutating that slice of the array, vue seems to work well with just the ref so all good on that front too! the open paths are relevant because they keep track of what someone has opened before and we should open that. For example lets say something expands the options "parent" > "child" > "grandchild" and then they collapse parent. When they expand parent they should see grandchild too, this is how the current treeselect does it and I think it is actually quite a helpful functionality especially in deeper structures

Ok, and that's going to use the 'open' prop which is retained rather than 'show' which just determines whether it's being displayed right now? Don't think I'd quite spotted the distinction there, but yeah that makes total sense.

EmmaLRussell · 2023-11-08T17:34:43Z

src/components/utils.ts

+    });
+};
+
+export const collapseOptions = (array: FlatOption[], optionPath: string[]) => {


Could do something similar here in terms of only examining/modifying the descendants of the optionsPath option..

EmmaLRussell · 2023-11-08T17:38:40Z

.gitignore

@@ -0,0 +1,23 @@
+# Logs


Probably don't need all of this I guess, but no harm!

yh this just came with vite XD thought i would just leave it in there

EmmaLRussell · 2023-11-08T17:46:18Z

src/style.css

@@ -0,0 +1,9 @@
+.dropdown-menu {
+  --cui-dropdown-link-active-bg: #ebedef !important;
+  --cui-dropdown-link-active-color: rbga(44, 56, 74, 0.95) !important;


We should probably document how package client can override style.

shall i leave this for another PR? i would like to add styling and configuration last once the single select, multiselect and search things are all done, ticket: https://mrc-ide.myjetbrains.com/youtrack/agiles/103-61/current?issue=mrc-4697

M-Kusumgar · 2023-11-09T15:45:13Z

Are you going to add a gha to run the tests and coverage?

Yes I will be adding these once single select and multiselect components are done, i think these things are better once theres a decent base!

…, emit modifiers, general clean up

EmmaLRussell

LGTM! Couple of tiny comments, but good to go.

EmmaLRussell · 2023-11-13T09:57:08Z

src/components/SingleSelect.vue

+  overflow-wrap: break-word;
+  text-align: left;
+}
+</style>../utils


What's this ../utils doing here?

EmmaLRussell · 2023-11-13T10:04:47Z

src/utils.ts

+
+    while (index < array.length && array[index].path.length > optionPath.length) {
+        const currentOption = array[index];
+        const show = openOptionsPaths.some(openOpPath => {


I think this bit merits a comment maybe something like.

Suggested change

const show = openOptionsPaths.some(openOpPath => {

// Show options whose immediate parents are marked as open

const show = openOptionsPaths.some(openOpPath => {

addressed this comment in #6, it is added in

M-Kusumgar added 2 commits November 2, 2023 17:30

add single select

6d880e6

set up modelValue and emit approach to single select

df8db0e

M-Kusumgar requested review from r-ash and EmmaLRussell November 2, 2023 18:08

M-Kusumgar added 7 commits November 2, 2023 18:13

simplify json stringify to .joins

90c3ba4

change expand and collapse chevrons to suit hint

2319d97

simplify toggle menu control

75d059c

better logic for path comparison

4e9d67c

simpler single select

663b50c

refactor single select

f8de279

improve utils

d8611c2

r-ash reviewed Nov 8, 2023

View reviewed changes

change path from strings with / to arrays

1f51b40

M-Kusumgar requested a review from r-ash November 8, 2023 14:36

EmmaLRussell reviewed Nov 8, 2023

View reviewed changes

M-Kusumgar added 2 commits November 9, 2023 15:49

emma changed: file structure, expand and collapse logic, package info…

e6320c4

…, emit modifiers, general clean up

remove unnecessary props from dropdown item

2a98e10

M-Kusumgar requested a review from EmmaLRussell November 9, 2023 15:53

remove debug

6ee3a1b

EmmaLRussell approved these changes Nov 13, 2023

View reviewed changes

M-Kusumgar marked this pull request as draft December 12, 2023 16:31

M-Kusumgar mentioned this pull request Dec 14, 2023

Mrc 4808 Add Base Select #4

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single Select #1

Single Select #1

M-Kusumgar commented Nov 2, 2023 •

edited

Loading

r-ash left a comment

EmmaLRussell left a comment

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 8, 2023

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 8, 2023

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 8, 2023

EmmaLRussell Nov 9, 2023 •

edited

Loading

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 8, 2023

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 8, 2023

EmmaLRussell Nov 8, 2023

EmmaLRussell Nov 9, 2023

M-Kusumgar Nov 9, 2023

EmmaLRussell Nov 8, 2023 •

edited

Loading

M-Kusumgar Nov 9, 2023

EmmaLRussell Nov 9, 2023

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 9, 2023

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 8, 2023

EmmaLRussell Nov 8, 2023

M-Kusumgar Nov 9, 2023

M-Kusumgar commented Nov 9, 2023

EmmaLRussell left a comment

EmmaLRussell Nov 13, 2023

EmmaLRussell Nov 13, 2023

M-Kusumgar Dec 14, 2023

		@@ -0,0 +1,65 @@
		import { FlatOption, Option } from "./types";

	const show = openOptionsPaths.some(openOpPath => {
	// Show options whose immediate parents are marked as open
	const show = openOptionsPaths.some(openOpPath => {

Single Select #1

Are you sure you want to change the base?

Single Select #1

Conversation

M-Kusumgar commented Nov 2, 2023 • edited Loading

Description

Guide

r-ash left a comment

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EmmaLRussell Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar commented Nov 9, 2023

EmmaLRussell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M-Kusumgar commented Nov 2, 2023 •

edited

Loading

EmmaLRussell Nov 9, 2023 •

edited

Loading

EmmaLRussell Nov 8, 2023 •

edited

Loading