Adds autocomplete components

[mperrotti]

Adds components that support adding autocomplete functionality to a text input.

Screenshots

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 43a84f4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/components	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	63.01 KB (+4.16% 🔺)
dist/browser.umd.js	63.39 KB (+4.21% 🔺)

[siddharthkp]

These are nit picks / tiny annoyances that I ran into while exploring the stories. I think both of these are polish and not deal breakers though.

 

1. Chrome tries to autofill the input which leads to 2 autocompletes at the same time. We might be able to turn the native one off with autocomplete="off"

   

 

2. When using keyboard navigation, I (wrongly?) expected to open the menu with Enter, but found this edge case instead where the focus is still on the last element and it can be toggled on/off with Enter

   

[jfuchs]

Overall: 😍 💅🏻 🤠 🎇

This is all great and very impressive. I have some questions for sure, but some of them might be mostly about my lack of context. Happy to jump on a call if that helps.

Unsolicited UX feedback

Another thing (already discussed over zoom) — it feels odd that hovering my mouse over the autocomplete suggestions would update the text input — I found that to be too much mutation to be driven off of a hover.

Something surprising: deleting a token results in the token text being put in the input.

Bug?

Not sure if it's a documentation bug or what but I noticed this odd text clipping:

CleanShot.2021-10-11.at.14.05.59.mp4

[jfuchs]

🤔 what's the use case for id as an external prop? I couldn't find a story or doc indicating why you'd want that.

[mperrotti]

I wanted to give an easy way to select elements for integration tests. If you think there are better ways to do this, I'm happy to get rid of this prop.

[jfuchs]

if a user doesn't pass an onSelectedChange, I think that means you'll be creating a new onSelectedChange with each render, and it looks like there's a useMemo call below with onSelectedChange in its dependencies array.

edit: same thing for filterFn

[mperrotti]

Good catch. Any ideas for how we could avoid the re-renders?

My first thought is to export the "default" functions and require engineers to explicitly pass a filterFn and onSelectedChange

[mperrotti]

I ended up just removing the assignments from the destructured props. If the prop is undefined, we just call the default functions defined up top.

However, I could still see a case for exporting getDefaultItemFilter and requiring a value for filterFn.

I'd love to hear your thoughts.

[jfuchs]

I think you can just change it to:

const AutocompleteMenu = ({
  onSelectedChange // (don't assign a default yet here)
}): Props => {
  // ...
  const onSelectedChangeOrDefault = useMemo(() => {
    return onSelectedChange || defaultOnSelectedChange(setInputValue)
  }, [...])
}

[mperrotti]

Ah ok, that works too.

Did you intentionally use useMemo instead of useCallback? I thought useCallback was the one for memoizing functions.

[mperrotti]

Not sure if it's a documentation bug or what but I noticed this odd text clipping

I hadn't seen that when working in Storybook, but I wasn't working with strings that long. I'll take a look.

Edit: I fixed the styling issue in the docs example. However, I noticed that the horizontal padding in the TextInput component causes some text clipping. We should file an issue.

[colebemis]

Noticed a few odds behaviors in the live code examples:

1. In the issue label example, the label colors reset when a new label is selected:

CleanShot.2021-10-12.at.15.06.05.mp4

2. In the multiple values example, selecting an item causes an error:

CleanShot.2021-10-12.at.15.09.35.mp4

[mperrotti]

Thanks @colebemis! I see what the problems are. Thankfully they're both just silly mistakes with the example code and not bugs in the components.

[colebemis]

Should we also export the props here?

[colebemis]

@jfuchs Can we right tests to ensure that components and their prop types are being exported?

[mperrotti]

Probably. It looks like we do it for other components.

[colebemis]

Looks great! ✨

A couple of things before we merge this:

• Add a changeset
• Export the prop types from the src/index.ts