Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TreeView: Implement typeahead #2357

Merged
merged 10 commits into from
Sep 26, 2022
Merged

TreeView: Implement typeahead #2357

merged 10 commits into from
Sep 26, 2022

Conversation

colebemis
Copy link
Contributor

@colebemis colebemis commented Sep 20, 2022

Summary

Implements typeahead search of items in a TreeView:

👉 Try it out

CleanShot.2022-09-20.at.16.47.30.mp4

Per @jsholes's suggestion, users can type multiple typeahead characters within a given timeout (300ms).

Test cases

Typeahead
    ✓ moves aria-activedescendant to the next item that matches the typed character (288 ms)
    ✓ does nothing if no items match the typed character (198 ms)
    ✓ supports multiple typed characters (268 ms)
    ✓ prioritizes items following the current aria-activedescendant (188 ms)
    ✓ wraps around to the beginning if no items match after the current aria-activedescendant (236 ms)

What should reviewers focus on?

  • Check that the typeahead behavior in the storybook stories works as expected
  • Check that the typeahead behaviors works well with screen readers
  • Review the typeahead implementation
  • Provide any feedback or missing functionality that you've noticed

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
Copy link

changeset-bot bot commented Sep 20, 2022

🦋 Changeset detected

Latest commit: a0b090c

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

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

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
Copy link
Contributor

github-actions bot commented Sep 20, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 77.1 KB (0%)
dist/browser.umd.js 77.71 KB (0%)

@colebemis colebemis temporarily deployed to github-pages September 20, 2022 23:54 Inactive
@colebemis colebemis marked this pull request as ready for review September 20, 2022 23:58
@colebemis colebemis requested review from a team and siddharthkp September 20, 2022 23:58
@colebemis colebemis temporarily deployed to github-pages September 21, 2022 00:08 Inactive
@colebemis colebemis temporarily deployed to github-pages September 21, 2022 16:39 Inactive
Copy link
Contributor

@mperrotti mperrotti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great!

Just two questions before I approve:

  1. Will useTypeahead still work if child nodes are loaded asynchronously when their parent directory is opened?
  2. Any idea how we could handle typeahead functionality if tree nodes are virtualized? It would be nice if I could still jump to a matching node even if it hasn't been loaded into the DOM yet.

onFocusChange: (element: Element) => void
}

export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

* wrapArray(['a', 'b', 'c', 'd'], 2) // ['c', 'd', 'a', 'b']
*/
function wrapArray<T>(array: T[], startIndex: number) {
return array.map((_, index) => array[(startIndex + index) % array.length])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. I love those rare moments where you get to use a modulo.

@colebemis
Copy link
Contributor Author

Will useTypeahead still work if child nodes are loaded asynchronously when their parent directory is opened?

Yeah, this implementation should work correctly with asynchronously loaded items. I will verify when I implement the async functionality 👍

Any idea how we could handle typeahead functionality if tree nodes are virtualized? It would be nice if I could still jump to a matching node even if it hasn't been loaded into the DOM yet.

Hmm. I haven't considered virtualization yet, but I imagine it will add a fair amount of complexity to the overall implementation. Is virtualization support in-scope for TreeView v1?

Co-authored-by: Josh Black <joshblack@github.com>
@colebemis colebemis temporarily deployed to github-pages September 21, 2022 21:04 Inactive
@mperrotti
Copy link
Contributor

@colebemis - we haven't discussed whether virtualization is in scope for v1.

I think we can move forward without virtualization support for v1. When/if it comes up we'll collaborate with product engineers to support virtualization.

Sound good?

@colebemis colebemis temporarily deployed to github-pages September 22, 2022 23:16 Inactive
Copy link
Contributor

@ericwbailey ericwbailey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so cool.

@colebemis colebemis temporarily deployed to github-pages September 24, 2022 16:56 Inactive
@colebemis colebemis merged commit 143286e into main Sep 26, 2022
@colebemis colebemis deleted the treeview-typeahead branch September 26, 2022 17:07
@primer-css primer-css mentioned this pull request Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants