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

feat(TreeView): add support for PageUp, PageDown #2546

Merged
merged 11 commits into from
Nov 10, 2022

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Nov 7, 2022

Add in support for PageUp and PageDown on TreeView. The way this works is that when a user presses PageDown while focus is within a TreeView we look for the first treeitem following the currently active item that is not currently in the viewport. If one exists, we focus it.

PageUp works the same way but in reverse, look for treeitem's before the current item and if one is currently not in the viewport focus it.

Approaches

  • Tried to move this into useRovingTabIndex but unfortunately useFocusZone will always preventDefault() on the PageUp, PageDown keys
  • List all items, find ones after the current item, focus the first one that isn't in the viewport
    • Challenge: it will bring the item to the bottom of screen when we likely want to pull it to the top
  • (Current approach) Run code at the end of a scroll event to find the first or last item in the viewport and focus is

@joshblack joshblack requested review from colebemis and a team November 7, 2022 22:05
@changeset-bot
Copy link

changeset-bot bot commented Nov 7, 2022

🦋 Changeset detected

Latest commit: 870904a

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

@joshblack joshblack temporarily deployed to github-pages November 7, 2022 22:12 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Looks great!

I think most of this code probably belongs in the useRovingTabIndex.ts file

src/TreeView/TreeView.tsx Outdated Show resolved Hide resolved
@colebemis
Copy link
Contributor

I checked out the branch locally, and this is the behavior I'm seeing:

CleanShot.2022-11-08.at.10.18.33.mp4

Looks like maybe a race condition between the focus change and the default PageUp/Down behavior?

@joshblack
Copy link
Member Author

@colebemis seeing that too on the stress test one, let me dig into it and also check on other browsers too where PageUp, PageDown may have different scroll behavior

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 78.87 KB (0%)
dist/browser.umd.js 79.52 KB (0%)

@joshblack joshblack temporarily deployed to github-pages November 8, 2022 18:44 Inactive
@joshblack
Copy link
Member Author

@colebemis just updated with a different approach, let me know what you think!

Quick cross-browser check:

Screen.Recording.2022-11-08.at.3.52.07.PM.mov

@joshblack joshblack temporarily deployed to github-pages November 8, 2022 22:22 Inactive
@joshblack
Copy link
Member Author

@colebemis thanks for the feedback! Updated to try another heuristic with keyup plus the scroll container note you brought up 👀

@joshblack joshblack temporarily deployed to github-pages November 9, 2022 19:26 Inactive
Copy link
Contributor

@colebemis colebemis left a comment

Choose a reason for hiding this comment

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

Works so well! ✨ Excellent job on the implementation. This ended up being a lot trickier than I expected.

@colebemis colebemis merged commit 8a8b1a4 into main Nov 10, 2022
@colebemis colebemis deleted the treeview-add-page-up-down branch November 10, 2022 14:18
@primer-css primer-css mentioned this pull request Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants