PageLayout: Use HTML5 landmark elements #1973
Conversation
because the dividers are only decorative
🦋 Changeset detectedLatest commit: 8106309 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 📦
|
| @@ -286,6 +286,7 @@ const Pane: React.FC<PageLayoutPaneProps> = ({ | |||
| const computedDividerWhenNarrow = dividerWhenNarrow === 'inherit' ? divider : dividerWhenNarrow | |||
| return ( | |||
| <Box | |||
| as="aside" | |||
There was a problem hiding this comment.
Waiting on an answer to this question about <aside>: https://github.com/github/primer/issues/782#issuecomment-1069606557
| @@ -114,7 +114,6 @@ const HorizontalDivider: React.FC<DividerProps> = ({variant = 'none', variantWhe | |||
| const {padding} = React.useContext(PageLayoutContext) | |||
| return ( | |||
| <Box | |||
| role="separator" | |||
There was a problem hiding this comment.
ooh interesting, what does removing this do?
There was a problem hiding this comment.
If doesn't have role= separator, should this have aria-hidden=true instead?
Made that exact change for ActionList.Divider: #1757
There was a problem hiding this comment.
Good callout @siddharthkp. I believe my recommendation was to use both role="presentation" and aria-hidden="true". @colebemis, I'll update the recommendation in the a11y review issue.
There was a problem hiding this comment.
Is this necessary to do on a div element?
There was a problem hiding this comment.
Without adding role or aria-hidden, it like looks divider already does not appear in the accessibility tree:
What do role and aria-hidden do for us in this case?
There was a problem hiding this comment.
Ooh because it's an empty div, it would probably just get "Ignored" in the tree
Based on @alliethu's recommendation in her accessibility review, the
PageLayoutcomponent now renders HTML5 landmark elements (header,aside,footer) to improve the navigation experience for people using assistive technologies (like screen readers).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.