Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 9 additions & 0 deletions frontends/main/src/app-pages/HomePage/HomePage.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,15 @@ describe("Home Page Carousel", () => {
test("Headings", async () => {
setupAPIs()

setMockResponse.get(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: It would be straightforward to give assertHeadings a maxLevel opt. That might be a more transparent approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

Eh, I guess that doesn't help here actually, since below we do test for h3

expect.stringContaining(urls.learningResources.list()),
[],
)
setMockResponse.get(
expect.stringContaining(urls.learningResources.featured()),
[],
)

renderWithProviders(<HomePage heroImageIndex={1} />)
await waitFor(() => {
assertHeadings([
Expand Down
17 changes: 17 additions & 0 deletions frontends/main/src/page-components/ResourceCard/ResourceCard.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,25 @@ const useResourceCard = (resource?: LearningResource | null) => {
}
}

type HeadingElement = "h1" | "h2" | "h3" | "h4" | "h5" | "h6"

const subheadingMap: Record<HeadingElement, number> = {
h1: 2,
h2: 3,
h3: 4,
h4: 5,
h5: 6,
h6: 6,
}

type ResourceCardProps = Omit<
LearningResourceCardProps,
"href" | "onAddToLearningPathClick" | "onAddToUserListClick"
> & {
condensed?: boolean
list?: boolean
headingLevel?: number
parentHeadingEl?: HeadingElement
}

/**
Expand All @@ -88,6 +101,7 @@ const ResourceCard: React.FC<ResourceCardProps> = ({
resource,
condensed,
list,
parentHeadingEl,
...others
}) => {
const {
Expand All @@ -106,6 +120,8 @@ const ResourceCard: React.FC<ResourceCardProps> = ({
: list
? LearningResourceListCard
: LearningResourceCard

const headingLevel = parentHeadingEl ? subheadingMap[parentHeadingEl] : 6
return (
<>
<CardComponent
Expand All @@ -116,6 +132,7 @@ const ResourceCard: React.FC<ResourceCardProps> = ({
onAddToUserListClick={handleAddToUserListClick}
inUserList={inUserList}
inLearningPath={inLearningPath}
headingLevel={headingLevel}
{...others}
/>
<SignupPopover anchorEl={anchorEl} onClose={handleClosePopover} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -291,4 +291,41 @@ describe("ResourceCarousel", () => {
await screen.findByText(resources.list.results[2].title)
expect(screen.queryByText(resources.list.results[1].title)).toBeNull()
})

it.each([
{ titleComponent: "h1", expectedLevel: 2 },
{ titleComponent: "h2", expectedLevel: 3 },
{ titleComponent: "h3", expectedLevel: 4 },
{ titleComponent: "h4", expectedLevel: 5 },
{ titleComponent: "h5", expectedLevel: 6 },
{ titleComponent: "h6", expectedLevel: 6 },
] as const)(
"Resource cards have headingLevel set to next level down for screen reader navigation",
async ({ titleComponent, expectedLevel }) => {
const config: ResourceCarouselProps["config"] = [
{
label: "Resources",
data: {
type: "resources",
params: { resource_type: ["course"] },
},
},
]
const { resources } = setupApis()
renderWithProviders(
<ResourceCarousel
titleComponent={titleComponent}
title="My Carousel"
config={config}
/>,
)

const titleHeading = await screen.findByRole("heading", {
name: resources.list.results[0].title,
})
expect(titleHeading.getAttribute("aria-level")).toBe(
String(expectedLevel),
)
},
)
})
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ type ResourceCarouselProps = {
/**
* Element type for the carousel title
*/
titleComponent?: React.ElementType
titleComponent?: "h1" | "h2" | "h3" | "h4" | "h5" | "h6"
titleVariant?: TypographyProps["variant"]
excludeResourceId?: number
}
Expand Down Expand Up @@ -302,28 +302,32 @@ const ResourceCarousel: React.FC<ResourceCarouselProps> = ({
>[]
}
>
{({ resources, childrenLoading, tabConfig }) => (
<CarouselV2 arrowsContainer={ref}>
{isLoading || childrenLoading
? Array.from({ length: 6 }).map((_, index) => (
<ResourceCard
isLoading
key={index}
resource={null}
{...tabConfig.cardProps}
/>
))
: resources
.filter((resource) => resource.id !== excludeResourceId)
.map((resource) => (
{({ resources, childrenLoading, tabConfig }) => {
return (
<CarouselV2 arrowsContainer={ref}>
{isLoading || childrenLoading
? Array.from({ length: 6 }).map((_, index) => (
<ResourceCard
key={resource.id}
resource={resource}
isLoading
key={index}
resource={null}
parentHeadingEl={titleComponent}
{...tabConfig.cardProps}
/>
))}
</CarouselV2>
)}
))
: resources
.filter((resource) => resource.id !== excludeResourceId)
.map((resource) => (
<ResourceCard
key={resource.id}
resource={resource}
parentHeadingEl={titleComponent}
{...tabConfig.cardProps}
/>
))}
</CarouselV2>
)
}}
</PanelChildren>
</TabContext>
</MobileOverflow>
Expand Down
14 changes: 11 additions & 3 deletions frontends/main/src/page-components/SearchDisplay/SearchDisplay.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,7 @@ interface SearchDisplayProps {
toggleParamValue: UseResourceSearchParamsResult["toggleParamValue"]
showProfessionalToggle?: boolean
setSearchParams: UseResourceSearchParamsProps["setSearchParams"]
resultsHeadingEl: React.ElementType
resultsHeadingEl: "h1" | "h2" | "h3" | "h4" | "h5" | "h6"
filterHeadingEl: React.ElementType
}

Expand Down Expand Up @@ -914,15 +914,23 @@ const SearchDisplay: React.FC<SearchDisplayProps> = ({
.fill(null)
.map((a, index) => (
<li key={index}>
<ResourceCard isLoading={isLoading} list />
<ResourceCard
isLoading={isLoading}
parentHeadingEl={resultsHeadingEl}
list
/>
</li>
))}
</PlainList>
) : data && data.count > 0 ? (
<PlainList itemSpacing={1.5}>
{data.results.map((resource) => (
<li key={resource.id}>
<ResourceCard resource={resource} list />
<ResourceCard
resource={resource}
parentHeadingEl={resultsHeadingEl}
list
/>
</li>
))}
</PlainList>
Expand Down
14 changes: 14 additions & 0 deletions frontends/ol-components/src/components/Card/Card.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,18 @@ describe("Card", () => {
expect(divOnClick).toHaveBeenCalled()
expect(window.location.hash).toBe("#two")
})

test("Card title has heading role and aria-level set for screen reader navigation", async () => {
renderWithTheme(
<Card>
<Card.Title role="heading" aria-level={2}>
Title
</Card.Title>
</Card>,
)
const titleHeading = screen.getByRole("heading", {
name: "Title",
})
expect(titleHeading.getAttribute("aria-level")).toBe("2")
})
})
45 changes: 37 additions & 8 deletions frontends/ol-components/src/components/Card/Card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import React, {
isValidElement,
CSSProperties,
useCallback,
AriaAttributes,
ReactElement,
} from "react"
import type { AriaRole, AriaAttributes } from "react"
import styled from "@emotion/styled"
import { theme } from "../ThemeProvider/ThemeProvider"
import { pxToRem } from "../ThemeProvider/typography"
Expand Down Expand Up @@ -135,6 +135,35 @@ const Title = styled(
]
})

const LinkableTitle = ({
title,
size,
}: {
title: TitleProps
size: Size | undefined
}) => {
const { role, "aria-level": ariaLevel, href, children, ...rest } = title

return (
<Title
data-card-link={!!href}
className="MitCard-title"
size={size}
href={href}
{...rest}
>
{/*
* The card titles are links, but we also want them to be visible as headings for accessibility.
* Setting the role on the Title component would make it invisible as a link to screen readers,
* so we include a span to set the role on instead.
*/}
<span role={role} aria-level={ariaLevel}>
{children}
</span>
</Title>
)
}

const Footer = styled.span`
display: block;
height: ${pxToRem(16)};
Expand Down Expand Up @@ -218,6 +247,7 @@ type CardProps = {
forwardClicksToLink?: boolean
onClick?: React.MouseEventHandler<HTMLElement>
as?: React.ElementType
role?: AriaRole
} & AriaAttributes

export type ImageProps = NextImageProps & {
Expand All @@ -231,7 +261,8 @@ type TitleProps = {
lines?: number
style?: CSSProperties
lang?: string
}
role?: AriaRole
} & AriaAttributes

type SlotProps = { children?: ReactNode; style?: CSSProperties }

Expand Down Expand Up @@ -270,6 +301,7 @@ const Card: Card = ({
size,
onClick,
forwardClicksToLink = false,
role,
...others
}) => {
let content,
Expand Down Expand Up @@ -315,6 +347,7 @@ const Card: Card = ({
className={allClassNames}
size={size}
onClick={handleClick}
role={role}
>
{content}
</Container>
Expand All @@ -327,6 +360,7 @@ const Card: Card = ({
className={allClassNames}
size={size}
onClick={handleClick}
role={role}
>
{image && (
// alt text will be checked on Card.Image
Expand All @@ -345,12 +379,7 @@ const Card: Card = ({
{info.children}
</Info>
)}
<Title
data-card-link={!!title.href}
className="MitCard-title"
size={size}
{...title}
/>
<LinkableTitle title={title} size={size} />
</Body>
<Bottom>
<Footer className="MitCard-footer" {...footer}>
Expand Down
14 changes: 14 additions & 0 deletions frontends/ol-components/src/components/Card/ListCard.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,4 +107,18 @@ describe("ListCard", () => {
expect(divOnClick).toHaveBeenCalled()
expect(window.location.hash).toBe("#two")
})

test("Card title has heading role and aria-level set for screen reader navigation", async () => {
renderWithTheme(
<ListCard>
<ListCard.Title role="heading" aria-level={2}>
Title
</ListCard.Title>
</ListCard>,
)
const titleHeading = screen.getByRole("heading", {
name: "Title",
})
expect(titleHeading.getAttribute("aria-level")).toBe("2")
})
})
Loading
Loading