TreeView: Add support for leading and trailing visuals #2383
Conversation
🦋 Changeset detectedLatest commit: 4787ccb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
size-limit report 📦
|
colebemis
changed the title
colebemis
requested review from
a team,
josepmartins,
ericwbailey and
mperrotti
docs/content/TreeView.mdx
Outdated
| </TreeView.LeadingVisual> | ||
| Avatar.tsx | ||
| <TreeView.TrailingVisual> | ||
| <StyledOcticon icon={DiffAddedIcon} color="success.fg" /> |
There was a problem hiding this comment.
Since these trailing visuals are used to visually communicate diff status, I'm pretty sure they should have an aria-label.
cc @ericwbailey
There was a problem hiding this comment.
That makes sense. How should the aria-label for the trailing visual impact the aria-label of the treeitem?
There was a problem hiding this comment.
Currently, the aria-label would be Avatar.tsx added. That seems correct but I'd love confirmation from @ericwbailey.
There was a problem hiding this comment.
I updated the docs and stories to include aria-label on these trailing visuals 👍
There was a problem hiding this comment.
Sorry to get to this after merging. I try to avoid using aria-label on non-interactive elements, given their mixed support. In the API guidance I am recommending content that uses both aria-hidden="true" and a sr-only class applied to it.
In addition to the mixed support concern, we're also going to be using the text string to construct aria-labeledby announcements for the relevant TreeView node.
| {childrenWithoutSubTree} | ||
| <Slots> | ||
| {slots => ( | ||
| // QUESTION: How should leading and trailing visuals impact the aria-label? |
There was a problem hiding this comment.
Which aria-label are you talking about?
I don't think we'll need content that is accessible to assistive tech for most trailing and leading visuals.
There was a problem hiding this comment.
I'm thinking about the aria-label for the element with role=treeitem.
| const DirectoryIcon = () => { | ||
| const {isExpanded} = React.useContext(ItemContext) | ||
| const icon = isExpanded ? FileDirectoryOpenFillIcon : FileDirectoryFillIcon | ||
| // TODO: Use correct color |
There was a problem hiding this comment.
I'll open a @primer/primitives PR with the folder color.
| @@ -10,63 +10,98 @@ description: A hierarchical list of items where nested items can be expanded and | |||
| ### File tree navigation without directory links | |||
There was a problem hiding this comment.
Could we get an example (and maybe a story?) showing how to use the isExpanded render prop to set different icons for expanded and collapsed states.
There was a problem hiding this comment.
Great idea 👍 Let me know if the docs I added make sense
Summary
Adds
TreeView.LeadingVisual,TreeView.TrailingVisual, andTreeView.DirectoryIconcomponents to support leading and trailing visuals:👉 Try it out
Open questions
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.