Skip to content

Commit

Permalink
♿️(course glimpse) put the course icon after the heading
Browse files Browse the repository at this point in the history
putting info related to the course glimpse before its heading is not
great for two reasons:

- with a screen reader, when navigate by headings on the page to jump
between each course glimpse, it makes users miss the info
- with a screen reader, when navigating in the content "normally" (with
down arrow), the first thing announced for a glimpse course is the icon
title, which is pretty misleading. We should hear the course name
instead.
  • Loading branch information
manuhabitela committed Feb 28, 2022
1 parent 8f9a5e0 commit 8ed6684
Show file tree
Hide file tree
Showing 3 changed files with 26 additions and 16 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ Versioning](https://semver.org/spec/v2.0.0.html).
- Improve React search suggestion field labels for screen readers.
- Removed usage of deprecated Sass division '/' operator in favor of
'math.div'.
- Reordered course glimpse text order in the DOM for better screen reader support.

### Fixed

Expand Down
11 changes: 10 additions & 1 deletion src/frontend/js/components/CourseGlimpse/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@ describe('components/CourseGlimpse', () => {
},
duration: '3 months',
effort: '3 hours',
icon: null,
icon: {
sizes: '60px',
src: '/thumbs/icon_small.png',
srcset: 'some srcset',
title: 'Some icon',
color: 'red',
},
id: '742',
organization_highlighted: 'Some Organization',
organization_highlighted_cover_image: {
Expand Down Expand Up @@ -51,6 +57,9 @@ describe('components/CourseGlimpse', () => {
</IntlProvider>,
);

// first text we encounter should be the title, so that screen reader users get it first
expect(container.textContent?.indexOf('Course 42')).toBe(0);

// The link that wraps the course glimpse should have no title as its content is explicit enough
expect(screen.getByRole('link')).not.toHaveAttribute('title');
// The course glimpse shows the relevant information
Expand Down
30 changes: 15 additions & 15 deletions src/frontend/js/components/CourseGlimpse/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,21 +46,6 @@ const CourseGlimpseBase = ({ context, course }: CourseGlimpseProps & CommonDataP
)}
</div>
<div className="course-glimpse__content">
{course.icon ? (
<div className="course-glimpse__icon">
<span className="category-badge">
{/* alt forced to empty string because it's a decorative image */}
<img
alt=""
className="category-badge__icon"
sizes={course.icon.sizes}
src={course.icon.src}
srcSet={course.icon.srcset}
/>
<span className="category-badge__title">{course.icon.title}</span>
</span>
</div>
) : null}
<div className="course-glimpse__wrapper">
<h3 className="course-glimpse__title" title={course.title}>
{course.title}
Expand All @@ -87,6 +72,21 @@ const CourseGlimpseBase = ({ context, course }: CourseGlimpseProps & CommonDataP
<span>{course.code || '-'}</span>
</div>
</div>
{course.icon ? (
<div className="course-glimpse__icon">
<span className="category-badge">
{/* alt forced to empty string because it's a decorative image */}
<img
alt=""
className="category-badge__icon"
sizes={course.icon.sizes}
src={course.icon.src}
srcSet={course.icon.srcset}
/>
<span className="category-badge__title">{course.icon.title}</span>
</span>
</div>
) : null}
<CourseGlimpseFooter context={context} course={course} />
</div>
</a>
Expand Down

0 comments on commit 8ed6684

Please sign in to comment.