TreeView: Add current prop
#2348
Conversation
Co-authored-by: Eric Bailey <[email protected]>
|
This PR is ready for review but let's not merge until we merge #2331 |
mperrotti
left a comment
mperrotti
left a comment
There was a problem hiding this comment.
current is set on the Button parent node, but it looks like aria-activedescendent is being set to the ID of the Button.tsx child node.
| toggle(event) | ||
| } | ||
| }} | ||
| sx={{ |
There was a problem hiding this comment.
The value of sx could get reallly big as we start to get more into the styling. Maybe we could consider moving the style code into it's own file or at least outside of the component function's scope.
(Non-blocking for this PR)
Hmm, I'm not able to reproduce that:
|
|
@colebemis - I can't reproduce that anymore either. I think I must have unintentionally clicked |
|
|
||
| // If current item exists, set it as the active descendant | ||
| if (currentItem) { | ||
| setActiveDescendant(currentItem.id) |
There was a problem hiding this comment.
Is it weird that aria-activedescendant updates on the parent, but aria-current does not move unless you manage it with state?
It seemed strange to me at first, but then I thought about it and was ok with it.
(non-blocking)
There was a problem hiding this comment.
Yeah, I think of aria-current more as a "default value" for aria-activedescandant rather than state that should be kept in sync
There was a problem hiding this comment.
I update the code comments to make that more clear 👍
| }) | ||
|
|
||
| it('initializes aria-activedescendant to the first item by default', () => { | ||
| it('initializes aria-activedescendant to the current item by default', () => { |
| boxShadow: (theme: Theme) => `0 0 0 2px ${theme.colors.accent.emphasis}` | ||
| }, | ||
| '[role=treeitem][aria-current=true] > &': { | ||
| bg: 'actionListItem.default.selectedBg', |
There was a problem hiding this comment.
This primitive looks tidied to the ActionList, I haven't seen many primitives like this so far in Primer. Is it temporary or any other reasoning behind using a component scoped primitive name?
There was a problem hiding this comment.
TreeView builds on ActionList styles so we're reusing some ActionList tokens/primitives here
Summary
Adds a
currentprop toTreeView.ItemandTreeView.LinkItemto indicate which item is current (e.g., matches the user's current URL).Behavior
👉 Try it out
currentitem in a tree will be expanded by defaultcurrentitem will be expanded by default if it contains sub-itemsaria-activedescendantwill be thecurrentitem (if there is acurrrentitem).CleanShot.2022-09-19.at.12.10.17.mp4
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.