Autocomplete Complete

[JustBarnt]

Before submitting the PR:

• Does your PR reference an issue? New Component Autocomplete
• Did you update and run tests before submission using npm run test?
• Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
• Did you update documentation related to your new feature or changes?

What does your PR address?

Still a work in progress.

Below are items that I have progress or completion on.

---

• - Link in alphabetical order
• - Renamed to Autocomplete
• - Reverted page documentation in favor of new DocShell when ready
• - Improve filteredValues()

• No search term present
• Search term present, do a fuzzy search against a list of options
• Some way to provide an array of values, then filter the list to only options that DON'T include this (scenario: remove suggestions for input chip values that have already been selected)
  ^ I'm referring to this as "exclude" filtering? I'm 100% open to a better name.

---

• Filtering with a whitelist included (Need to watch video on setting up tests, but I did quick test myself.)
• Use Skeletons Navigation Styles ( I believe I have achieved this) as well as pulling out styles that were originally inline.
• updated .item to .autocomplete-item
• Created autoComplete action that controls the show/hiding of the list items
• Started work on getting keyboard functionality to work, I will need some guidance on this as it is very rough still. (Grabbed the base functionality from Popup.ts)

TODO

• Learn vitest
• Selecting an item and then click in the input again, shows the list pre-filtered because the input has value, to me this would not be the expected functionality. So I need to come up with a solution.

@endigo9740 If you get a quick chance to look at this- its much appreciated. I just want to make sure filterValues() is working more inline with what you were describing above.

Mainly the excludes filtering is what I am calling it: My interpretation is if the user has an array of values, uses the excludes filtering and they search foo, the popup below the input would update and show the user "bar and fizz" because foo is already typed in the input.

Excludes Example

Foo is not present in list, nor is FooBar (this could be considered a bug, but I think should be easily fixable if regex is used compared to includes?)

---

IF I'm correct in that functionality I need some help figuring out how to keep removing from the list as they type. I was thinking would allow or allow users to specify separators to indicate when to search for a new string?

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
skeleton-docs	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Mar 22, 2023 at 4:51PM (UTC)

[jameslounds]

Selecting an item and then click in the input again, shows the list pre-filtered because the input has value, to me this would not be the expected functionality. So I need to come up with a solution.

What worked for me (in my interim implementation until this is released) was clearing the input when it's clicked, and selecting the text in the input on keyboard focus. I've considered adding a listener for Command + Z to put the last selected value back.

MUI doesn't consider a filter to be active when you've clicked an option and go back on the input
Mantine leaves the input alone on click, but selects the text in the input when focussed with the keyboard

[endigo9740]

Hey Brent, so what you had was great. A lot of the core concepts are perfect. There's just some odds and ends that didn't quite align with how I was expecting this to work. Given the number of changes I hope you don't mind me committing these alterations direction. Sometimes it's just easier to show than tell!

I've noted everything in detail below. You should be able to pull the changes down and review locally.

HTML/CSS

• Modified the HTML template to use a native <select> element for all the a11y benefits that provides
• Styles are now set via .select (which can be replaced in element to go "headless")

Logic

• We don't want to control the visibility of this component, that's for the users. We're building primitives, remember, which means folks can drop this into a Skeleton Popup utility and do everything you built with the power of Floating UI!
• Given this I've disabled your Action - it's super cool, but redundant! I've not deleted it in case you want to grab it and save it for later use in your own projects!
• I've refactored the filter logic to make it a bit more "DRY" and modular. Still operates in the same fashion.
• I've adjusted prop input to semantically represent the input value (your target input)
• I've adjusted prop options to semantically represent the list of objects that populate the select > objects
• The mode type is simple enough we can set that inline, removing the need for types.ts
• I've modified the custom dispatched event to be selection so it doesn't overlap with native events.
• Finally on:selection returns the entire object row, so users can utilize any key/value pair from within this

Remaining Tasks

• I think the whitelist you've instituted might be a bit too open ended. Reading the entire data set as a string is too loose. I might recommend you test each array row independently using some()
• The select[size] element is a bit more rigid in height. We'll need to find a clever way to adjust the height automatically based on result length, while still specifying a max height. I'm open to ideas.
• We do lose the ability to hit use keyboard navigation to drop from the input into the autocomplete, but tab should still work. Maybe this is where we keep the Action as an optional supliment?
• Fill free to nix the commented line items if you agree with their removal

Feel free to ping me on Discord if you want to discuss this in detail. I'm also down to jump on a quick Zoom call if that's easier! But let me know what you think about this direction. I think it offers a ton of flexibility without trying to reinvent the wheel where we don't need to.

[jameslounds]

I’m interested to hear why the choice of a select element for the actual input, rather than a hidden one like in InputChip. On Safari (especially mobile), the styling doesn’t apply, and I’m not sure if this behaviour is intended.

The select also causes the filter to be reapplied, and the currently focussed element is selected, making the third item in the dropdown often unreachable.

I'm getting really excited watching this PR come together, great job!

RPReplay_Final1677620457.mp4

In the video below, I use the arrow keys, but would not be able to select Fizz only using the arrow keys

Screen.Recording.2023-02-28.at.21.47.39.mp4

[endigo9740]

I’m interested to hear why the choice of a select element for the actual input, rather than a hidden one like in InputChip. On Safari (especially mobile), the styling doesn’t apply, and I’m not sure if this behaviour is intended.

The idea here is the autocomplete is a supplementary UI, not the form state itself. The choice in Select is primarily for presentation and a11y benefits that comes from native inputs.

The select also causes the filter to be reapplied, and the currently focussed element is selected, making the third item in the dropdown often unreachable.

Hmm, not a fan of how that's handled on your mobile device. That's really frustrating. I can't tell from the UI, is that an iPad or Android tablet? If it's the latter, which browser?

[jameslounds]

Hmm, not a fan of how that's handled on your mobile device. That's really frustrating. I can't tell from the UI, is that an iPad or Android tablet? If it's the latter, which browser?

iPad. Also looks a little janky, but better on desktop safari.

I think this is just an annoying limitation webkit imposes (again). Other UI libraries seem not to use a select, but this may be for another reason I haven't considered

[endigo9740]

iPad. Also looks a little janky, but better on desktop safari.
I think this is just an annoying limitation webkit imposes (again). Other UI libraries seem not to use a select, but this may be for another reason I haven't considered

Yeah the UI presentation is the simplest part of all this, so honestly if this is an issue we can change that out. The more important bit is that the data mapping and filtering logic are working as intended.

[JustBarnt]

@endigo9740 Another draft update!

I think the whitelist you've instituted might be a bit too open ended. Reading the entire data set as a string is too loose. I might recommend you test each array row independently using some()

---

• Whitelist searching has been refactored based on what you talked about above, hopefully this is more inline with what you are envisioning.

As we discussed, I changed the select back to a div as we were experience issues with the default way select elements acted.

With that change, I have added what I think should be the correct aria-labeling for the items that make up the autocomplete component itself. I left the select options in, but commented out in-case you wanted to take a stab at seeing if you can figure out a way to get the select element to work opposite of how it should.

Remaining

• a11y items
• Keyboard navigaion
• Doc creation
• Test's
• Styling

After this review, assuming everything is okay- These items should be able to be started!

[jameslounds]

Something really tiny, but I think you've misunderstood how tabindex works. 0 and -1 are the only values you'll typically need to use. Setting it to 0 adds the element to the tab index, making it possible to tab focus the element in the order it appears in the DOM.
Setting them incrementally makes the first option focus as expected, then everything else that is tabbable on the page after the autocomplete, then the browser controls (address bar etc), then the second option in the list (with tabindex 1).
If there were 2 or more Autocomplete components on the page, the first option would tab for each as above, then the address bar etc, then the second option for each component (tabindex=1), then the third option for each (tabindex=2).

https://www.a11yproject.com/posts/how-to-use-the-tabindex-attribute/

[JustBarnt]

Something really tiny, but I think you've misunderstood how tabindex works. 0 and -1 are the only values you'll typically need to use. Setting it to 0 adds the element to the tab index, making it possible to tab focus the element in the order it appears in the DOM. Setting them incrementally makes the first option focus as expected, then everything else that is tabbable on the page after the autocomplete, then the browser controls (address bar etc), then the second option in the list (with tabindex 1). If there were 2 or more Autocomplete components on the page, the first option would tab for each as above, then the address bar etc, then the second option for each component (tabindex=1), then the third option for each (tabindex=2).

https://www.a11yproject.com/posts/how-to-use-the-tabindex-attribute/

Ah yes, thank you! I will get that fixed real quick!

[endigo9740]

@JustBarnt I've done another pass through this. So a few things here:

1. Within ismatch we can just use Object.values(obj) to find the values. Much less code needed.
2. I agree with dropping the dedicated functions, I've nuked those comments
3. I've reimplemented the design using our list-nav element styles, paired with some class styles
4. As part of this a max height is now set via a Tailwind utility class to enable scrolling if the list is too long
5. I've disabled the popup - currently the input is losing focus when the popup is triggered. We'll need a new popup event type to resolve this. Some work is occurring here that might help

Additionally I've found another bug. Currently if you search for bar you should get both Bar and FooBar as suggested options. But this is not occurring:

I'll leave that to you to try and track down and resolve.

Also just general rule of thumb - no need commenting things for preserving the history, that's what version control is for. If you're not using something, just remove it! If something is temporarily disabled though, comments are fine (ala the popup)

[JustBarnt]

Additionally I've found another bug. Currently if you search for bar you should get both Bar and FooBar as suggested options.

@endigo9740 I know exactly why that's happening- to me its a bug, as I would not expect to get Foobar when searching bar, because foo- comes before it.

if (value.substring(0, input.length) === input)

This is why, I'm checking if the sub-string of characters in value is equal to input, so foobar wouldn't match because it would be checking if foo === bar.

But, I can remove that if in this case then. Haha

[JustBarnt]

@endigo9740 Completed the first draft of doc writing. Looking forward to your critique as I'd like to improve my technical writing.

• Removed conditional that prevented bar from including foobar in its results.
• Wrote first draft of docs.

---

Questions

I noticed a couple of things I have questions on.

1. I noticed you added an {@html v.label} for displaying the button text, I was curious as too why you did that instead of just {v.label}
2. prunedRestProps is not longer being called in the parent container for removing class from any restProps being passed. Was that intended or just missed when converting the nav list?
3. I now get a warning on nav or ul with role="listbox"` which is":
   • A11y: Non-interactive element <ul | nav> cannot have interactive role 'listbox'
     Not sure what needs to be done for that unfortunately so I'm looking to you to see if that's a valid thing, or if VSCode is just on something haha.
4. Lastly I wanna propose passing the ref to the input element to the Autocomplete itself and create an onSelection event inside the autocomplete that handles setting the value of the input its passed. This would prevent having to use multiple event handlers outside the Autocomplete if the user has multiple Autocomplete components on the same page. Also less they have to write that could potentially cause issues.

[Hugos68]

Width of the demo input needs to be adjusted to be responsive for mobile devices

[endigo9740]

With the latest changes @JustBarnt and I have agreed this is ready to merge. Great job Brent!

Just in case anyone else is wondering, we WILL be pairing this with the Popup feature, but we need some adjustments to the action's trigger events and focus before this will work properly. We'll then also aim to improve the keyboard interaction.