Skip to content

PageLayout: Use HTML5 landmark elements#1973

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.

Sign up for GitHub

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

Merged
colebemis merged 7 commits into from
Mar 23, 2022
Merged

PageLayout: Use HTML5 landmark elements #1973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
89dabaf
Use HTML5 landmark elements
colebemis Mar 17, 2022
dbc9f0b
Remove separator role
colebemis Mar 17, 2022
3d3511d
Create brown-maps-yawn.md
colebemis Mar 17, 2022
6637d4f
Update snapshot tests
colebemis Mar 17, 2022
31f105e
Update component checklist
colebemis Mar 17, 2022
c30240e
Merge branch 'page-layout-a11y' of github.com:primer/react into page-…
colebemis Mar 17, 2022
8106309
Merge branch 'main' into page-layout-a11y
colebemis Mar 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/brown-maps-yawn.md
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

The `PageLayout` component now renders [HTML5 landmark elements](https://web.dev/use-landmarks/) (`header`, `aside`, `footer`) to improve the navigation experience for people using assistive technologies (like screen readers)
2 changes: 1 addition & 1 deletion docs/content/PageLayout.mdx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -258,7 +258,7 @@ See [storybook](https://primer.style/react/storybook?path=/story/layout-pagelayo
usageExamplesDocumented: true,
hasStorybookStories: true,
designReviewed: false,
a11yReviewed: false,
a11yReviewed: true,
stableApi: false,
addressedApiFeedback: false,
hasDesignGuidelines: false,
Expand Down
6 changes: 4 additions & 2 deletions src/PageLayout/PageLayout.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -114,7 +114,6 @@ const HorizontalDivider: React.FC<DividerProps> = ({variant = 'none', variantWhe
const {padding} = React.useContext(PageLayoutContext)
return (
<Box
role="separator"

@pksjcepksjceMar 17, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh interesting, what does removing this do?

@siddharthkpsiddharthkpMar 17, 2022•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If doesn't have role= separator, should this have aria-hidden=true instead?

Made that exact change for ActionList.Divider: #1757

@alliethualliethuMar 17, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@colebemiscolebemisMar 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary to do on a div element?

@colebemiscolebemisMar 17, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without adding role or aria-hidden, it like looks divider already does not appear in the accessibility tree:

CleanShot 2022-03-17 at 16 25 07@2x

What do role and aria-hidden do for us in this case?

@siddharthkpsiddharthkpMar 18, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh because it's an empty div, it would probably just get "Ignored" in the tree

// eslint-disable-next-line @typescript-eslint/no-explicit-any
sx={(theme: any) =>
merge<BetterSystemStyleObject>(
Expand DownExpand Up@@ -156,7 +155,6 @@ const verticalDividerVariants = {
const VerticalDivider: React.FC<DividerProps> = ({variant = 'none', variantWhenNarrow = 'inherit', sx = {}}) => {
return (
<Box
role="separator"
// eslint-disable-next-line @typescript-eslint/no-explicit-any
sx={(theme: any) =>
merge<BetterSystemStyleObject>(
Expand DownExpand Up@@ -191,6 +189,7 @@ const Header: React.FC<PageLayoutHeaderProps> = ({
const {rowGap} = React.useContext(PageLayoutContext)
return (
<Box
as="header"
sx={merge<BetterSystemStyleObject>(
{
order: REGION_ORDER.header,
Expand DownExpand Up@@ -230,6 +229,7 @@ const contentWidths = {
const Content: React.FC<PageLayoutContentProps> = ({width = 'full', children, sx = {}}) => {
return (
<Box
as="main"
sx={merge<BetterSystemStyleObject>(
{
order: REGION_ORDER.content,
Expand DownExpand Up@@ -286,6 +286,7 @@ const Pane: React.FC<PageLayoutPaneProps> = ({
const computedDividerWhenNarrow = dividerWhenNarrow === 'inherit' ? divider : dividerWhenNarrow
return (
<Box
as="aside"

@colebemiscolebemisMar 17, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on an answer to this question about <aside>: https://github.com/github/primer/issues/782#issuecomment-1069606557

// eslint-disable-next-line @typescript-eslint/no-explicit-any
sx={(theme: any) =>
merge<BetterSystemStyleObject>(
Expand DownExpand Up@@ -344,6 +345,7 @@ const Footer: React.FC<PageLayoutFooterProps> = ({
const {rowGap} = React.useContext(PageLayoutContext)
return (
<Box
as="footer"
sx={merge<BetterSystemStyleObject>(
{
order: REGION_ORDER.footer,
Expand Down
80 changes: 32 additions & 48 deletions src/PageLayout/__snapshots__/PageLayout.test.tsx.snap
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -154,50 +154,46 @@ exports[`PageLayout renders condensed layout 1`] = `
<div
class="c1"
>
<div
<header
class="c2"
>
Header
<div
class="c3"
role="separator"
/>
</div>
<div
</header>
<main
class="c4"
>
<div
class="c5"
>
Content
</div>
</div>
<div
</main>
<aside
class="c6"
>
<div
class="c7"
role="separator"
/>
<div
class="c8"
role="separator"
/>
<div
class="c9"
>
Pane
</div>
</div>
<div
</aside>
<footer
class="c10"
>
<div
class="c7"
role="separator"
/>
Footer
</div>
</footer>
</div>
</div>
</div>
Expand DownExpand Up@@ -403,50 +399,46 @@ exports[`PageLayout renders default layout 1`] = `
<div
class="c1"
>
<div
<header
class="c2"
>
Header
<div
class="c3"
role="separator"
/>
</div>
<div
</header>
<main
class="c4"
>
<div
class="c5"
>
Content
</div>
</div>
<div
</main>
<aside
class="c6"
>
<div
class="c7"
role="separator"
/>
<div
class="c8"
role="separator"
/>
<div
class="c9"
>
Pane
</div>
</div>
<div
</aside>
<footer
class="c10"
>
<div
class="c7"
role="separator"
/>
Footer
</div>
</footer>
</div>
</div>
</div>
Expand DownExpand Up@@ -652,50 +644,46 @@ exports[`PageLayout renders pane in different position when narrow 1`] = `
<div
class="c1"
>
<div
<header
class="c2"
>
Header
<div
class="c3"
role="separator"
/>
</div>
<div
</header>
<main
class="c4"
>
<div
class="c5"
>
Content
</div>
</div>
<div
</main>
<aside
class="c6"
>
<div
class="c3"
role="separator"
/>
<div
class="c7"
role="separator"
/>
<div
class="c8"
>
Pane
</div>
</div>
<div
</aside>
<footer
class="c9"
>
<div
class="c10"
role="separator"
/>
Footer
</div>
</footer>
</div>
</div>
</div>
Expand DownExpand Up@@ -936,50 +924,46 @@ exports[`PageLayout renders with dividers 1`] = `
<div
class="c1"
>
<div
<header
class="c2"
>
Header
<div
class="c3"
role="separator"
/>
</div>
<div
</header>
<main
class="c4"
>
<div
class="c5"
>
Content
</div>
</div>
<div
</main>
<aside
class="c6"
>
<div
class="c7"
role="separator"
/>
<div
class="c8"
role="separator"
/>
<div
class="c9"
>
Pane
</div>
</div>
<div
</aside>
<footer
class="c10"
>
<div
class="c11"
role="separator"
/>
Footer
</div>
</footer>
</div>
</div>
</div>
Expand Down