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 Backspace to collapse a folder #2504

Merged
merged 9 commits into from
Nov 2, 2022

Conversation

joshblack
Copy link
Member

@joshblack joshblack commented Oct 31, 2022

Based on feedback here, this PR adds in support for Backspace to collapse a folder.

Testing & Reviewing

You can verify that Backspace works by visiting a story for TreeView and expanding a folder. Pressing Backspace on the folder should not collapse it. If a parent item is available, focus should move to it. If the element is at the root, focus should not move.

Screen.Recording.2022-10-31.at.4.00.02.PM.mov

@joshblack joshblack requested review from a team and mperrotti October 31, 2022 19:37
@changeset-bot
Copy link

changeset-bot bot commented Oct 31, 2022

🦋 Changeset detected

Latest commit: 4070655

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 Oct 31, 2022

size-limit report 📦

Path Size
dist/browser.esm.js 78.87 KB (+0.06% 🔺)
dist/browser.umd.js 79.51 KB (+0.06% 🔺)

@joshblack joshblack temporarily deployed to github-pages October 31, 2022 19:44 Inactive
@colebemis
Copy link
Contributor

When inside of a folder, pressing Backspace on an item should noop.

I think pressing Backspace on an item should focus the parent folder. That's how I interpreted @jscholes' description here: https://github.com/github/primer/issues/1114#issuecomment-1292397344

So, in generic terms, Backspace should: move focus to the parent node of the currently focused node, without changing the expanded state of any nodes. If the currently focused node is a root node, it should do nothing, even if that root node is an expanded parent.

@jscholes
Copy link

Correct, @colebemis. @joshblack Backspace shouldn't collapse any nodes. It should move focus to the parent of the focused node, or do nothing if the focused node is a root node. But the state of nodes shouldn't change.

@joshblack
Copy link
Member Author

joshblack commented Oct 31, 2022

Totally makes sense! Making those changes now 👍 Sorry about the confusion over behavior on my end.

@joshblack joshblack temporarily deployed to github-pages October 31, 2022 21:07 Inactive
package.json Outdated Show resolved Hide resolved
@joshblack joshblack temporarily deployed to github-pages November 1, 2022 15:05 Inactive
@colebemis
Copy link
Contributor

Wanted to call out this note from @jscholes:

Critical note: Backspace on Windows and Delete on macOS are not the same key, even though they occupy the same physical position on most keyboards. This functionality must only apply to Backspace, and not Delete.

In the storybook, the Delete key on my mac keyboard is triggering the backspace behavior. I'm not sure how we can programmatically differentiate Backspace on Windows and Delete on macOS. Any ideas @jscholes @joshblack?

@jscholes
Copy link

jscholes commented Nov 1, 2022

@colebemis / @joshblack Does this only apply on Mac? I.e. does ['Backspace', 'ArrowLeft', 'ArrowRight', 'ArrowUp', 'ArrowDown', 'Enter'].includes(event.key) resolve to true when pressing Delete specifically on Windows? If not, I think we should be fine; WebKit may just treat them as the same. Not sure what we should do down the line if we actually want to support node deletion though...

@joshblack
Copy link
Member Author

@jscholes great question, I tried testing with the Delete key on Windows 10 real quick and it seems like event.key will have the value Delete and so that condition will end up being false.

As you noted, it does seem like on macOS it treats the delete key when in that classic Backspace position as Backspace in JavaScript

@jscholes
Copy link

jscholes commented Nov 1, 2022

@joshblack I think Fn+Backspace on Mac is Delete specifically. Backspace on its own is Backspace, so the current state should be good.

Co-authored-by: Cole Bemis <colebemis@github.com>
@joshblack joshblack temporarily deployed to github-pages November 1, 2022 22:03 Inactive
@joshblack joshblack enabled auto-merge (squash) November 2, 2022 16:04
@joshblack joshblack temporarily deployed to github-pages November 2, 2022 16:11 Inactive
@joshblack joshblack merged commit ea4a3c2 into main Nov 2, 2022
@joshblack joshblack deleted the feat/add-backspace-key-to-tree branch November 2, 2022 16:12
@primer-css primer-css mentioned this pull request Nov 2, 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.

3 participants