Add multiselect to combobox

[rolfheij-sil]

---

This change is 

[Sebastian-ubs]

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

  if (Array.isArray(value)) {
    if (value.length === 0) return buttonPlaceholder;
    return value.map((entry) => getOptionLabel(entry)).join(', ');

In addition to the concatenation of the value (that will soon run out of space), we need another functional parameter that will define the label based on the array input (if provided). Alternatively buttonPlaceholder could be made a function that takes the value as input.

[jolierabideau]

Thanks Rolf, this is super useful! Just curious if the UX design for ComboBox is that they are always multi-select and we should use a different component like Select in other places? I'm working on the new settings design and need a dropdown for the projects that is searchable but not multi-select. I was imagining we could add flags to the ComboBox props, and put different functionality like multi-select and clearable behind them.

[Sebastian-ubs]

Your changes looks good.

I am just wondering if this is the right component to use, or if we need a different filter component that re-assembles the popover and trigger separately.
As in the mockups I can see elements that look more like a shadcn dropdown menu.

the popover having a header and a button at the end

hierarchy

categories / sections to separate the list + using a simple button as trigger

Reviewed 5 of 7 files at r1, all commit messages.
Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

[Sebastian-ubs]

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @rolfheij-sil)

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 135 at r1 (raw file):

                    'tw-opacity-0': Array.isArray(value)
                      ? !value.includes(option)
                      : !value || getOptionLabel(value) !== getOptionLabel(option),

please check if this mode still works for keyboard only selection or breaks this

[rolfheij-sil]

@jolierabideau Thanks! :) I though about adding a isMultiSelect bool or something to the combobox, but I figured that when you assign an array to the value prop, you want multiselect, and if you assign a single value to it, you don't want multiselect. I don't know what UX wants here. For what you need, you could just use the ComboBox with a non-array as the value, right?

@Sebastian-ubs Thanks :) For the first two of your screenshots it seems reasonable to use the dropdown menu component, but for the last one (and the Type menu which is very similar in design) I think this combobox is a good candidate.

Reviewable status: 5 of 7 files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

Previously, Sebastian-ubs wrote…

In addition to the concatenation of the value (that will soon run out of space), we need another functional parameter that will define the label based on the array input (if provided). Alternatively buttonPlaceholder could be made a function that takes the value as input.

I don't understand what you mean by another functional parameter that will define the label based on the array input

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 135 at r1 (raw file):

Previously, Sebastian-ubs wrote…

please check if this mode still works for keyboard only selection or breaks this

Yep, works perfectly :)

[Sebastian-ubs]

Partially agree.
The mock up shows

• the trigger being a button with icon and static text and no arrows
• a headline list entry
  Which I can't see in the combobox.

Although the mock up looks different it states "filter by type of resource", which works well with acombobox.
Just a quick confirmation with Alex/Allison could be helpful.

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @rolfheij-sil)

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I don't understand what you mean by another functional parameter that will define the label based on the array input

a new parameter of type function e.g. "getButtonLabel" (and rename the internal func). So that consumers could implement e.g. a function that shows "Checks (5 selected)".
But maybe even more options are needed, like stated above.

[jolierabideau]

@rolfheij-sil That makes sense! Yes, I think that would work

[rolfheij-sil]

Thanks for the feedback. I added three new props:

• a prop that lets you hide the Chevrons
• a prop that lets you select if you want the values to show up on the button or always want the placeholder to show
• a prop that lets you add an icon on the left.

I think it is still reasonable to keep all of this inside the same component, instead of having multiple very similar combobox-like components in PBR.

Reviewable status: 5 of 7 files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 71 at r1 (raw file):

Previously, Sebastian-ubs wrote…

a new parameter of type function e.g. "getButtonLabel" (and rename the internal func). So that consumers could implement e.g. a function that shows "Checks (5 selected)".
But maybe even more options are needed, like stated above.

Ah yes, now I understand. I suggest to not add this new feature right now. It's not needed for what I am currently building.

[rolfheij-sil]

Added one more prop that lets you control how the popover menu should be aligned. By default it's center aligned, but I see it's start aligned for all the design you posted above @Sebastian-ubs

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @Sebastian-ubs)

[Sebastian-ubs]

You are right @rolfheij-sil, we should not end up with a lot of very similar components, but also adding a lot of properties to a single component increases the testing matrix and may make usage inconsistent and harder to understand for our users.
One goal of the component library is to have simple components with clearly distinct variances.
Therefore spotting a missing variant could also mean to go back to the designers and say "we do not have this right now. Are you sure we should add this new variant?"
So I suggest talking to agreeing on the component to use before building too many options in or creating similar components.

Reviewable status: 0 of 7 files reviewed, all discussions resolved

[rolfheij-sil]

Yeah, I get what you're saying. I was thinking the other way around though: If I see a UX design, I assume that is what they want me to build, and add new component/variants to achieve that.
This could be something to talk about in the next UX+Dev meeting: a more high level discussion on how to convert a UX design into real code.

For now: Do you want to talk more about this specific component, or can we add these changes to PBR?

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

[Sebastian-ubs]

Thank you Rolf. I guess the ux design might not have considered with which component this should be implemented. I know that in the past we had agreed to mostly use standard shadncn components and if deviating from it, make sure what the change is. The checks and home/open design however seem to have forgotten about this.
You might have even noticed that also I have contradicted my previous message and only remembered we want less variance later.

But maybe even more options are needed, like stated above.

So, yes it will be helpful to reiterate on this in the ux-dev meeting.
In general I want to review out components and define which to use when together with Jolie (but unfortunately did not recognize this would be needed now, so did not do it yet).

For this case I would suggest to implement the bare minimum, so that we might not match the design shown, but only the functionality and then discuss it in the ux-dev or as a follow up.
That would mean to remove the newly added

• a prop that lets you hide the Chevrons
• a prop that lets you select if you want the values to show up on the button or always want the placeholder to show
• a prop that lets you add an icon on the left.

Reviewable status: 0 of 7 files reviewed, all discussions resolved

[rolfheij-sil]

Thanks! I'll wait with removing those (or not) until we've talked about it in the UX+Dev meeting. That'll save me some time :)

Reviewable status: 0 of 7 files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

[irahopkinson]

My 2 cents. You could keep the complicated component as the internal implementation detail and only export several wrapper components that simplify the options.

[Sebastian-ubs]

I've done some small adjustments based on latest design decision. You may want to pull/merge 46ab2b4 into yours.

[Sebastian-ubs]

There is also some other small differences which I had previously done on this branch as also can be seen in this v0 - version3:

• add a border-primary around the multiselect combobox when not all or none are selected + for the search bar, when a search term is present
• remove color of the star icon
• combobox: show the selected value as placeholder, when only 1 was selected
• change "All resources" to "Any resource type"
• allow to clear the last selected language (but not the last selected type)

[rolfheij-sil]

I've finalized the implementation of the multi-select combo box. It's ready to be merged now, so that it can be used in the updated Get Resources design on 1290-v2-resources-and-home-extensions

Reviewable status: 0 of 18 files reviewed, all discussions resolved (waiting on @Sebastian-ubs)

[Sebastian-ubs]

Thank you very much. I added some non-blocking thoughts and few blocking comments

Reviewed 7 of 17 files at r5, 1 of 8 files at r6, 10 of 10 files at r7, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @rolfheij-sil)

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 4 at r7 (raw file):

import { Check, ChevronsUpDown, Star } from 'lucide-react';
import { ReactNode, useCallback, useMemo, useState } from 'react';
import { Button } from '../shadcn-ui/button';

should import from @components/shadcn-ui/button
Same for the 2 imports below

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 23 at r7 (raw file):

interface MultiSelectComboBoxProps {
  options: MultiSelectComboBoxEntry[];
  getOptionsCount?: (option: MultiSelectComboBoxEntry) => number;

should we rename those to "entries" and "getEntriesCount"?

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 53 at r7 (raw file):

  );

  const getPlaceholderText = () => {

Does this work well for all the cases? Or should we rather export this function?
So in this case definitively the user has to track selected items when they want to show something like "x types" right?

One issue I see - that was in the Filter design Figma mock ups, but not in the ones I added for hand off - is that the Placeholder should show up as placeholder text-muted, but a selection should show up in text-primary. Where currently all shows in primary.

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 88 at r7 (raw file):

          className={cn(
            'tw-w-full tw-justify-between',
            selected.length > 0 && selected.length < options.length && 'tw-border-primary',

I think this should be optional for a general multi select combobox. Could you add prop for it or should we rename this whole component to something like filter-combobox?

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):

          <CommandInput placeholder={`Search ${placeholder.toLowerCase()}...`} />
          <CommandList>
            <CommandEmpty>No item found.</CommandEmpty>

This is not localizable.

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 28 at r7 (raw file):

  /**
   * The selected value(s) that the combo box currently holds. Must be shallow equal to one or more
   * of the options entries.

adjust back to no multi select

---

lib/platform-bible-react/src/components/shadcn-ui/badge.tsx line 32 at r7 (raw file):

function Badge({ className, variant, ...props }: BadgeProps) {
  return <div className={cn('pr-twp', badgeVariants({ variant }), className)} {...props} />;

It could be nice to include the x button already.
Might be done by adding a onClear prop that would if exists implicitly add a clear button or by explicitly stating both.

[Sebastian-ubs]

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 53 at r7 (raw file):

One issue I see
Rethinking this, it may actually work to change to text-muted with inverting the same logic as below
selected.length > 0 && selected.length < options.length

[Sebastian-ubs]

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 53 at r7 (raw file):

One issue I see

Rethinking this, it may actually work to change to text-muted with inverting the same logic as below

selected.length > 0 && selected.length < options.length

[rolfheij-sil]

Thanks for reviewing

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Sebastian-ubs)

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 4 at r7 (raw file):

Previously, Sebastian-ubs wrote…

should import from @components/shadcn-ui/button
Same for the 2 imports below

Done

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 23 at r7 (raw file):

Previously, Sebastian-ubs wrote…

should we rename those to "entries" and "getEntriesCount"?

Done

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 53 at r7 (raw file):

Previously, Sebastian-ubs wrote…

One issue I see

Rethinking this, it may actually work to change to text-muted with inverting the same logic as below

selected.length > 0 && selected.length < options.length

Done, I added to logic to have none/all selected placeholder show up muted

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 88 at r7 (raw file):

Previously, Sebastian-ubs wrote…

I think this should be optional for a general multi select combobox. Could you add prop for it or should we rename this whole component to something like filter-combobox?

I don't know. Let's just leave it in here for now. If people don't like this, we can always refactor this out into a prop. But I don't want to overcomplicate this component with extra props without good reason

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):

Previously, Sebastian-ubs wrote…

This is not localizable.

Can we just leave this out then? Or maybe display some icon? I don't think now is the right time to start thinking about localizing basic react components.

---

lib/platform-bible-react/src/components/basics/combo-box.component.tsx line 28 at r7 (raw file):

Previously, Sebastian-ubs wrote…

adjust back to no multi select

Done

---

lib/platform-bible-react/src/components/shadcn-ui/badge.tsx line 32 at r7 (raw file):

Previously, Sebastian-ubs wrote…

It could be nice to include the x button already.
Might be done by adding a onClear prop that would if exists implicitly add a clear button or by explicitly stating both.

The work here is already suffering from a lot of scope creep, let's keep this just as basic as we can. Also we don't generally change shadcn components unless we have to.

[Sebastian-ubs]

Reviewed 9 of 9 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @rolfheij-sil)

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 88 at r7 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

I don't know. Let's just leave it in here for now. If people don't like this, we can always refactor this out into a prop. But I don't want to overcomplicate this component with extra props without good reason

fine

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):
the combobox is exporting a prop

  commandEmptyMessage = 'No option found',

[rolfheij-sil]

Reviewable status: 12 of 18 files reviewed, 2 unresolved discussions (waiting on @Sebastian-ubs)

---

lib/platform-bible-react/src/components/advanced/multi-select-combo-box.tsx line 106 at r7 (raw file):

Previously, Sebastian-ubs wrote…

the combobox is exporting a prop

  commandEmptyMessage = 'No option found',

Done. Added the prop

[Sebastian-ubs]

Reviewed 6 of 6 files at r9, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

lib/platform-bible-react/src/components/shadcn-ui/badge.tsx line 32 at r7 (raw file):

Previously, rolfheij-sil (Rolf Heij) wrote…

The work here is already suffering from a lot of scope creep, let's keep this just as basic as we can. Also we don't generally change shadcn components unless we have to.

I agree, we could follow up on that later. If to change them or not is an open question still.

[Sebastian-ubs]

hurray. Thank you for all the effort you put into this @rolfheij-sil