-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Speed up tree navigation by returning to parent when collapsing… #9547
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -701,6 +701,26 @@ Tree items may also be links to another page or website. This can be achieved by | |||
|
|
||||
| The `<TreeItem>` component works with frameworks and client side routers like [Next.js](https://nextjs.org/) and [React Router](https://reactrouter.com/en/main). As with other React Aria components that support links, this works via the <TypeLink links={docs.links} type={docs.exports.RouterProvider} /> component at the root of your app. See the [client side routing guide](routing.html) to learn how to set this up. | ||||
|
|
||||
| ## Keyboard navigation | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question to the team, do we want a prop for this, or should this just be the default for all trees?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems reasonable to make it the default behavior. do we even need this section if that is the case? edit: i guess it could be nice to set up the expectations of what the keyboard navigation is especially if it has changed |
||||
|
|
||||
| By default, pressing the collapse key (<Keyboard>←</Keyboard> in LTR, <Keyboard>→</Keyboard> in RTL) on an expanded item will collapse it. The key will do nothing on non-collapsible items. The same key is used to navigate between the actions within tree items. | ||||
|
|
||||
| The `shouldNavigateToCollapsibleParent` prop enables a faster navigation behavior: when the collapse key is pressed on a leaf item or an already collapsed parent, focus moves to that item's parent. This helps users quickly navigate up the tree without needing to manually navigate to each parent item. But it has a trade-off: users can no longer use that key to cycle through actions on the current item. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't really accurate, you can still use the arrow keys to cycle through actions, it's only if the focus is on the row, it won't wrap to the end of the actions. But if you're among the actions already, then it works just fine.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think maybe the concern is that people might expect that they be able to continue cycling through by pressing the left arrow even when the row is collapsed but now, it'll take you to the parent instead because that was the original behavior. It's true that the keyboard navigation for actions work correctly if you're already on them but it's what happens afterwards that has changed. Of course, if we want this to be the default behavior, then this will have to change because we can't both cycle through the actions and change focus to the parent.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will need to remove this reference as well |
||||
|
|
||||
| ```tsx example | ||||
| <FileTree | ||||
| shouldNavigateToCollapsibleParent | ||||
| selectionMode="single" | ||||
| defaultExpandedKeys={['1', '2']} | ||||
| defaultSelectedKeys={['5']} | ||||
| /> | ||||
| ``` | ||||
|
|
||||
| With this prop enabled: | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'm fine to have the bulleted list, should it move above the example? |
||||
| - Pressing collapse on a leaf item moves focus to its parent | ||||
| - Pressing collapse on an expanded item collapses it | ||||
| - Pressing collapse again on a collapsed item moves focus to its parent | ||||
|
Comment on lines
+721
to
+723
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does "pressing" mean here? clicking on the arrow with your mouse, the keyboard arrow keys? |
||||
|
|
||||
| ## Disabled items | ||||
|
|
||||
| A `TreeItem` can be disabled with the `isDisabled` prop. This will disable all interactions on the item | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just make this behavior the default? Is there a reason to make it optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to impede on the existing behaviour that cycles through item actions. My preference is absolutely to make this behaviour the default (and even not to have a prop for it).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also of the opinion it should be the default, no prop needed
#9547 (comment)