feat(components): Autocomplete Multiselect

[ZakaryH]

Motivations

Multiselect on Autocomplete is something we want to offer. it becomes a replacement for MultiSelect, Chips (dismissible) and overall can meet UX needs that Combobox is not suited for.

The particular case we have in mind right now is a team selection UX.

the foundations and bones for multiselect are already in place, it's a matter of sorting out some remaining details, polishing the experience for this flow, and adding a few new features that increase the usability of the component when selecting multiple.

Changes

Added

• clearable prop (this has a known issue that is true for all inputs that focusing the clear button with a "tab" will cause the input's onBlur to fire)
• multiple prop properly implemented
• customRenderValue prop that can be used to customize the appearance of a selected value (multiple only)
• default UI for the selected items on multiselect
• arrow navigation between those selected items to make it easier for a keyboard user to have the same granularity in removing selections
• when input is empty, pressing backspace will remove the most recent selection
• disabled and readonly apply to the selections as expected

Changed

• height of each item is now stabilized to prevent shifts between the checkmark Icon being rendered or not rendered
• active index (highlighting) logic updated to work with multiselect
• swapped some useEffect hooks for a more event-driven approach to make it easier to folow

Deprecated

Removed

Fixed

snuck in a fix to a single character deletion bug that can result in your options getting out of sync due to the debounce.

Security

Testing

set up some multiple Autocompletes with various combinations including customizing the selection display, play around with it and make sure you like how it feels and behaves!

Changes can be
tested via Pre-release

---

In Atlantis we use Github's built in pull request reviews.

[ZakaryH]

this is an existing issue. our height isn't stable causing visible shifts when something is selected, it's just much easier to see in multiselect.

another way to deal with this would be to set a min or fixed height on the row but that gets interesting with custom content.

[cloudflare-workers-and-pages]

Deploying atlantis with    Cloudflare Pages

Latest commit:	0d83ea5
Status:	✅  Deploy successful!
Preview URL:	https://1aa34e0e.atlantis.pages.dev
Branch Preview URL:	https://autocomplete-multiselection.atlantis.pages.dev

View logs

[ZakaryH]

e2e test failures are expected. from stabilizing the height, each row now contributes more to the overall height and it's enough to trigger the diff threshold.

[ZakaryH]

clearable looks quite bad on this branch. I broke out the fix for that in a separate PR to try and reduce the noise. ideally our visual tests catch any differences that spawn from the change that will make this work nicely here as well as everywhere else.

#2925

[ZakaryH]

@nad182 finally got CI green. here's the PR.

[nad182]

I might be wrong, but to me all the screenshots look the same (before/after). Not sure if that is expected. I only see that the "after" ones are 1px shorter (in height).

[nad182]

Never mind. I left a comment on the commit, where there was no difference between the "deleted" and "added" screenshot. But I see the difference in the diff from master.

[nad182]

There are some issues in Storybook that I noticed:

1. "With customRenderValue" example: both the close icon (X) and chevron (^) are visible next to each other. Might not be the best UI.
2. If there are multiple autocompletes on one page (close to each other), the close icon has a higher z-index, so it appears "on the" top of another Autocomplete's dropdown/menu.
3. Chevron doesn't change direction (not sure if that is expected, but I thought it would).

If you think it would be better to address those in the follow-up (or if they are not an issue), feel free to merge this PR!

[MichaelParadis]

@nad182 the z-index is a known issue https://jobber.atlassian.net/browse/JOB-128115

As for the cheveron changing direction it looks to be related it being a custom suffix with it as a chevron down (the default autocomplete doesn’t have the chevron.

I do think the spacing between the suffix and clearable should be fixed because from what I can see it isn't using a customRenderInput (if it were I would expect the consumer to handle the styling since they are doing custom stuff)

[ZakaryH]

1. yeah that's an existing issue that comes from FormField and happens on every input, so to solve that we have to solve it for everything. the relevant properties are: size: large clearable and suffix the combination of the three results in this.

3. z-index issue is also existing and applies to all FormFields/inputs

4. that would be something for the usage to implement via onOpen and onClose, though our mockup doesn't do that, it just stays as a downward facing chevron

I was trying to use large to make it so that the addition of the custom values doesn't cause a visual shift in the height, but that doesn't actually work. there's more to it for large to properly offer more space. I don't believe this is a requirement for now so I'd like to follow up with that, especially since there's this other issue. I'll update the story to not use large so it doesn't look as bad as ticket the issues.

[AutumnDaweBaillie]

Overall design review I'd say this is mostly g2g considering the other issues called out above are known issues that apply to all fields, so I think those are fine to sort out outside of this.

I say mostly because I honestly totally missed the chevron staying the same direction (good callout!!) but after a quick sanity check around other libraries it does look like most change directions between the open and close state and that just makes sense for those icons in general (chevrons should point the direction they're going) so I think that would be a good thing for us to include as well. Otherwise this LGTM!

[ZakaryH]

I say mostly because I honestly totally missed the chevron staying the same direction (good callout!!) but after a quick sanity check around other libraries it does look like most change directions between the open and close state and that just makes sense for those icons in general (chevrons should point the direction they're going) so I think that would be a good thing for us to include as well. Otherwise this LGTM!

fair enough. I'll add an example, though we won't have as much control as we might like to say use an animated rotation rather than a more abrupt icon swap.

oh wait, whoops I neglected to implement onOpen and onClose 😬

ok added those, and an example now.

[edison-cy-yang]

Commenting on the commits that happened after Jacob's approval, without getting all the context on previous commits, they look good to me.

Only thing that caught my eyes is the naming of limitSelectionText now that it returns a ReactNode. Maybe I'm missing some context but it's not very clear to me what the prop should do based on its name.

[ZakaryH]

Only thing that caught my eyes is the naming of limitSelectionText now that it returns a ReactNode. Maybe I'm missing some context but it's not very clear to me what the prop should do based on its name.

that's fair. do you think the JSDocs are sufficient? or should I add a note saying that it must resolve to a string/text or that it must be phrasing content?

the reason for accepting a ReactNode is to allow this

limitSelectionText={<Trans>Hola Amigo</Trans>}

where the Trans component translates it to another language, but returns it inside an inert fragment so the final result is still valid

<Typography>
// at runtime this would be <>Hello friend</>
 {props.limitSelectionText(truncatedCount)
</Typography>

[ZakaryH]

@edison-cy-yang forgot to tag you on the above

[edison-cy-yang]

Only thing that caught my eyes is the naming of limitSelectionText now that it returns a ReactNode. Maybe I'm missing some context but it's not very clear to me what the prop should do based on its name.

that's fair. do you think the JSDocs are sufficient? or should I add a note saying that it must resolve to a string/text or that it must be phrasing content?

the reason for accepting a ReactNode is to allow this

limitSelectionText={<Trans>Hola Amigo</Trans>}

where the Trans component translates it to another language, but returns it inside an inert fragment so the final result is still valid

<Typography>
// at runtime this would be <>Hello friend</>
 {props.limitSelectionText(truncatedCount)
</Typography>

I think adding a note saying it needs to resolve to phrasing content would be great.

[ZakaryH]

@edison-cy-yang added some additional context in the JSdoc