TreeView: Implement markup #2305
Conversation
🦋 Changeset detectedLatest commit: 3f3ab76 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
requested review from
a team,
rezrah,
ericwbailey,
joshblack and
mperrotti
and removed request for
rezrah
| } | ||
|
|
||
| // TODO: Use an <a> element to enable native browser behavior like opening links in a new tab | ||
| const LinkItem: React.FC<TreeViewLinkItemProps> = ({href, onSelect, ...props}) => { |
There was a problem hiding this comment.
We're not using a <a> element here based on a recommendation from @jscholes. Later, let's investigate whether there's an accessible way to use an <a> element so we don't lose out on native browser link features (like opening links in a new tab).
There was a problem hiding this comment.
Oh interesting! I wonder what the accessibility concerns with <a> are?
There was a problem hiding this comment.
@broccolinisoup - it's not the <a> itself, it's the markup that would be required to make it work. It would be easier to explain in a Loom or on a Zoom call/screenshare if you want the details.
There was a problem hiding this comment.
Looks great! Sharing the checklist that I used below, the only thing that I noticed was the label for the group of items (which I think you have as a TODO). Also was curious if the intent was for aria-current was to be used or not in the navigation case.
Checklist
-
role="navigation",<nav>- Contains the
role="tree"element if its treeitem elements are links - Has an accessible label describing the landmark content
- Contains the
-
role="tree"1- Container uses
role="tree" - Every descendant of
role="tree"hasrole="treeitem"
- Container uses
-
role="treeitem"2- Contained in an element with
role="tree"orrole="group" - Expandable collection of
treeitemelements are enclosed in an element withrole="group" -
role="treeitem"usesaria-expandedto communicate expansion - Click target for expansion should not be focusable
- A group of expandable treeitem elements should be
hiddenordisplay: nonewhen not expanded - A group of expandable treeitem elements is
aria-labelledbyby the label of therole="treeitem"element - A navigable treeitem element should communicate if its the current element within the group with
aria-current="page"
- Contained in an element with
Questions
- Is there a good place to learn more about the
aria-levelrequirement? It's something that I've always avoided if the markup can communicate the level
Footnotes
|
@joshblack Thanks for such a thorough review! ❤️
Yup, I left a TODO comment about this. I plan to address this when adding the LeadingVisual and TrailingVisual components because they'll impact how we generate the treeitem label.
I'm also planning to address this in a future PR. I'd like to implement the aria-current prop and associated styles in one PR.
I was following the guidance in the TreeView API spec put together by @mperrotti and @ericwbailey: https://github.com/github/primer/pull/1273/files. They might know more about |
There was a problem hiding this comment.
Markup looks correct based on our discussions with @jscholes and @ericwbailey 👍
I'm really hoping we can come back to this and figure out how to make <a> work so we don't lose native browser link features 🤞
| } | ||
|
|
||
| // TODO: Use an <a> element to enable native browser behavior like opening links in a new tab | ||
| const LinkItem: React.FC<TreeViewLinkItemProps> = ({href, onSelect, ...props}) => { |
There was a problem hiding this comment.
@broccolinisoup - it's not the <a> itself, it's the markup that would be required to make it work. It would be easier to explain in a Loom or on a Zoom call/screenshare if you want the details.
This PR implements the bare bones of the TreeView component. My focus for this PR is to get TreeView rendering accessible markup (as defined by the TreeView spec).
Acceptance criteria
We can merge this PR when we're happy with the markup rendered in these TreeView storybook stories:
Screenshots
Merge checklist
Next steps