Skip to content
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.

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

fix(PageHeader): add role prop and aria-label in top-level element #4956

5 changes: 5 additions & 0 deletions .changeset/fifty-rockets-joke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": minor
---

fix(PageHeader): add role prop and aria-label in top-level element
4 changes: 2 additions & 2 deletions packages/react/src/Hidden/Hidden.examples.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const setViewportParamToNarrow = {
}
export const Webhooks = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Webhooks">
<PageHeader.ContextArea>
<PageHeader.ParentLink href="http://github.com">Repository settings</PageHeader.ParentLink>
</PageHeader.ContextArea>
Expand Down Expand Up @@ -51,7 +51,7 @@ WebhooksOnNarrowViewport.parameters = setViewportParamToNarrow

export const PullRequestPage = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Add Hidden utility component">
<PageHeader.ContextArea>
<PageHeader.ParentLink href="http://github.com">Pull requests</PageHeader.ParentLink>
</PageHeader.ContextArea>
Expand Down
12 changes: 8 additions & 4 deletions packages/react/src/PageHeader/PageHeader.dev.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ export default meta

export const LargeVariantWithMultilineTitle = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader
role="banner"
aria-label="Title long title some extra loooong looong words here some extra loooong looong words here some extra loooong
looong words here some extra loooong looong words here some extra loooong looong words here"
>
<PageHeader.LeadingAction>
<IconButton aria-label="Edit" icon={PencilIcon} variant="invisible" />
</PageHeader.LeadingAction>
Expand Down Expand Up @@ -46,7 +50,7 @@ export const LargeVariantWithMultilineTitle = () => (

export const ArrayTypeFontSizeOnTitle = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Issue Title">
<PageHeader.TitleArea>
<PageHeader.Title
sx={{
Expand All @@ -64,7 +68,7 @@ export const ArrayTypeFontSizeOnTitle = () => (

export const ThemeBaseFontSizeOnTitle = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Issue Title">
<PageHeader.TitleArea>
<PageHeader.Title
sx={{
Expand All @@ -80,7 +84,7 @@ export const ThemeBaseFontSizeOnTitle = () => (

export const StringTypeFontSizeOnTitle = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Issue Title">
<PageHeader.TitleArea>
<PageHeader.Title
sx={{
Expand Down
7 changes: 6 additions & 1 deletion packages/react/src/PageHeader/PageHeader.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
"name": "aria-label",
"type": "string | undefined",
"defaultValue": "",
"description": "A unique label for the rendered main landmark"
"description": "A unique label for the rendered landmark"
},
{
"name": "className",
Expand All @@ -24,6 +24,11 @@
"defaultValue": "false",
"description": "Whether the content is hidden."
},
{
"name": "role",
"type": "AriaRole",
"description": "The ARIA role to assign to the top-level node of this component."
},
{
"name": "sx",
"type": "SystemStyleObject"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ const setViewportParamToNarrow = {
}
export const Webhooks = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Webhooks">
<PageHeader.TitleArea>
<PageHeader.Title as="h2">Webhooks</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -78,7 +78,10 @@ WebhooksOnNarrowViewport.parameters = setViewportParamToNarrow

export const PullRequestPage = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader
role="banner"
aria-label="PageHeader component initial layout explorations extra long pull request title"
>
<PageHeader.TitleArea>
<PageHeader.Title as="h1">
PageHeader component initial layout explorations extra long pull request title
Expand Down Expand Up @@ -146,7 +149,7 @@ PullRequestPageOnNarrowViewport.parameters = setViewportParamToNarrow

export const FilesPage = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="PageHeader.tsx">
<PageHeader.TitleArea sx={{alignItems: 'center'}}>
<Text sx={{color: 'rgb(101, 109, 118)', fontSize: '14px', fontWeight: 'normal'}}>/</Text>
<PageHeader.Title as="h1" sx={{fontSize: '14px', height: '21px'}}>
Expand Down
24 changes: 12 additions & 12 deletions packages/react/src/PageHeader/PageHeader.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const meta: Meta = {

export const HasTitleOnly = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -41,7 +41,7 @@ export const HasTitleOnly = () => (

export const HasLargeTitle = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea variant="large">
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -51,7 +51,7 @@ export const HasLargeTitle = () => (

export const WithLeadingAndTrailingVisuals = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.LeadingVisual>
<GitPullRequestIcon />
Expand All @@ -67,7 +67,7 @@ export const WithLeadingAndTrailingVisuals = () => (

export const WithLeadingVisualHiddenOnRegularViewport = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.LeadingVisual hidden={{regular: true}}>
<GitPullRequestIcon />
Expand All @@ -89,7 +89,7 @@ WithLeadingVisualHiddenOnRegularViewport.parameters = {

export const WithActions = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -107,7 +107,7 @@ export const WithActions = () => (

export const WithDescriptionSlot = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Add-pageheader-docs">
<PageHeader.TitleArea>
<PageHeader.Title>add-pageheader-docs</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -125,7 +125,7 @@ export const WithDescriptionSlot = () => (

export const WithNavigationSlot = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Pull request title">
<PageHeader.TitleArea>
<PageHeader.Title>Pull request title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -151,7 +151,7 @@ export const WithNavigationSlot = () => (

export const WithCustomNavigation = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Pull request title">
<PageHeader.TitleArea>
<PageHeader.Title>Pull request title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -173,7 +173,7 @@ export const WithCustomNavigation = () => (

export const WithLeadingAndTrailingActions = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -189,7 +189,7 @@ export const WithLeadingAndTrailingActions = () => (

export const WithParentLinkAndActionsOfContextArea = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -215,7 +215,7 @@ WithParentLinkAndActionsOfContextArea.parameters = {

export const WithContextBarAndActionsOfContextArea = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand Down Expand Up @@ -250,7 +250,7 @@ WithContextBarAndActionsOfContextArea.parameters = {
}
export const WithActionsThatHaveResponsiveContent = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Webhooks">
<PageHeader.TitleArea>
<PageHeader.Title as="h2">Webhooks</PageHeader.Title>
</PageHeader.TitleArea>
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/PageHeader/PageHeader.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ export default meta

export const Playground: StoryFn = args => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader aria-label={args.Title} role="banner">
<PageHeader.TitleArea
variant={{
narrow: args['Title.variant'],
Expand Down Expand Up @@ -287,7 +287,7 @@ export const Playground: StoryFn = args => (

export const Default = () => (
<Box sx={{padding: 3}}>
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand Down
52 changes: 46 additions & 6 deletions packages/react/src/PageHeader/PageHeader.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ describe('PageHeader', () => {
Component: PageHeader,
options: {skipAs: true, skipSx: true},
toRender: () => (
<PageHeader>
<PageHeader role="banner" aria-label="Banner">
<PageHeader.TitleArea></PageHeader.TitleArea>
<PageHeader.ContextArea></PageHeader.ContextArea>
<PageHeader.Description></PageHeader.Description>
Expand Down Expand Up @@ -138,7 +138,7 @@ describe('PageHeader', () => {
})
it('respects the title variant prop', () => {
const {getByText} = render(
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea variant="large">
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -149,7 +149,7 @@ describe('PageHeader', () => {
})
it('renders "aria-label" prop when Navigation is rendered as "nav" landmark', () => {
const {getByLabelText, getByText} = render(
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -161,9 +161,9 @@ describe('PageHeader', () => {
expect(getByLabelText('Custom')).toBeInTheDocument()
expect(getByText('Navigation')).toHaveAttribute('aria-label', 'Custom')
})
it('does not renders "aria-label" prop when Navigation is rendered as "div"', () => {
it('does not render "aria-label" prop when Navigation is rendered as "div"', () => {
const {getByText} = render(
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -175,7 +175,7 @@ describe('PageHeader', () => {
it('logs a warning when the Navigation component is rendered as "nav" but no "aria-label" or "aria-labelledby" prop is provided', () => {
const consoleSpy = jest.spyOn(global.console, 'warn').mockImplementation()
render(
<PageHeader>
<PageHeader role="banner" aria-label="Title">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
Expand All @@ -186,4 +186,44 @@ describe('PageHeader', () => {

consoleSpy.mockRestore()
})
it('does not render "role" attribute when not explicitly specified', () => {
const {container} = render(
<PageHeader>
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
</PageHeader>,
)
expect(container.firstChild).not.toHaveAttribute('role')
})
it('renders "role" attribute when explicitly specified', () => {
const {container} = render(
<PageHeader role="banner">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
</PageHeader>,
)
expect(container.firstChild).toHaveAttribute('role', 'banner')
})
it('does not render "aria-label" attribute when not explicitly specified', () => {
const {container} = render(
<PageHeader role="banner">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
</PageHeader>,
)
expect(container.firstChild).not.toHaveAttribute('aria-label')
})
it('renders custom "aria-label" attribute when explicitly specified', () => {
const {container} = render(
<PageHeader aria-label="Custom aria-label" role="banner">
<PageHeader.TitleArea>
<PageHeader.Title>Title</PageHeader.Title>
</PageHeader.TitleArea>
</PageHeader>,
)
expect(container.firstChild).toHaveAttribute('aria-label', 'Custom aria-label')
})
})
13 changes: 11 additions & 2 deletions packages/react/src/PageHeader/PageHeader.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../uti
import {getBreakpointDeclarations} from '../utils/getBreakpointDeclarations'
import {warning} from '../utils/warning'
import {useProvidedRefOrCreate} from '../hooks'
import type {AriaRole} from '../utils/types'

const GRID_ROW_ORDER = {
ContextArea: 1,
Expand Down Expand Up @@ -63,10 +64,11 @@ export type PageHeaderProps = {
'aria-label'?: React.AriaAttributes['aria-label']
as?: React.ElementType | 'header' | 'div'
className?: string
role?: AriaRole
} & SxProp

const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeaderProps>>(
({children, className, sx = {}, as = 'div'}, forwardedRef) => {
({children, className, sx = {}, as = 'div', 'aria-label': ariaLabel, role}, forwardedRef) => {
const rootStyles = {
display: 'grid',
// We have max 5 columns.
Expand Down Expand Up @@ -158,7 +160,14 @@ const Root = React.forwardRef<HTMLDivElement, React.PropsWithChildren<PageHeader
[children, rootRef],
)
return (
<Box ref={rootRef} as={as} className={className} sx={merge<BetterSystemStyleObject>(rootStyles, sx)}>
<Box
ref={rootRef}
as={as}
className={className}
sx={merge<BetterSystemStyleObject>(rootStyles, sx)}
aria-label={ariaLabel}
role={role}
>
{children}
</Box>
)
Expand Down
Loading
Loading