-
Notifications
You must be signed in to change notification settings - Fork 3
Card Accessibility Improvements #1778
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
Conversation
| * - an anchor or button element | ||
| * - OR an element with data-card-action | ||
| */ | ||
| forwardClicksToLink?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally, I made this the default behavior. However, since reliably NOT triggering the link when clicking interactive children requires some effort from consumer (adding data-card-action if the interactive child is not a anchor or button) I decided to make it opt-in to increase visibility.
| position: absolute; | ||
| bottom: 16px; | ||
| right: 16px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we used absolute positioning because:
- The "main" card element was an anchor
- anchors can't have interactive children (invalid HTML)
- so we made anchor + buttons children of a wrapper
Since the main card element is no longer an anchor, this is no longer necessary.
| <StyledCard href={href} className={className} size={size}> | ||
| <StyledCard | ||
| as="article" | ||
| aria-label={`${readableType}: ${resource.title}`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this label? I'm finding that Voiceover repeats the title. Is the<a> content overridable with aria-label to add the readableType
This only happens on the small Media carousel cards, but not the Featured Courses carousel, though I don't see any difference in the HTML that would explain why/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need the label... Otherwise the landmarks menu does not show meaningful info:

And landmarks are supposed to have labels:
A visible label should be provided, referenced with aria-labelledby. If necessary, brief, descriptive, label can be provided with aria-label.
That said... Apparently (I just learned) article is not a real landmark role, but MDN's article role page says:
Articles are not considered a navigational landmark, but many assistive technologies that support landmarks also support a means to navigate among articles. They may also support indication of nesting relationships within articles.
I guess VoiceOver just treats them as landmarks.
Anyway: The usage here is based on what we do in OCW. (Although there we have infinite scroll, se we also add aria-posinset.
| // alt text will be checked on Card.Image | ||
| // eslint-disable-next-line styled-components-a11y/alt-text | ||
| <Image | ||
| className="MitCard-image" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not one for this PR as this hasn't changed here, but can we use data-testids if these are just for selection in the tests (or ideally avoid altogether). .MitCard-root and .MitCard-info are the only ones in use for CSS selectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do use MitCard-root and MitCard-info for styling at least once, which is why they were added. When I added them, I added for other parts (like MitCard-title) that made sense but weren't actually used (as of yet).
jonkafton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just the one comment with repeated content for the small resources cards, though not sure if this is controlloable or a VoiceOver artifact as the HTML looks the same.
7a71953 to
4317e8e
Compare
What are the relevant tickets?
Closes https://github.com/mitodl/hq/issues/5538
Description (What does it do?)
This PR makes some accessibility enhancements to cards and learning resource cards in particular.
Card Title is Link: Previously the entire card was a link. Now just the title is a link, leading to much better link names. The whole card remains clickable.
LearningResourceCards are
articles: Learning resource cards now usearticleas a root element with[resource.type]: [resource.title]as the article name. Consequently, cards are landmarks that appear in landmark navigationSimplified Positioning: Cards no longer use absolute positioning for buttons.
Screenshots
Card aesthetics should be unchanged:
How can this be tested?
/dashboard/my-lists//dashboard/my-lists/{list_id}when "Reordering"2.Where cards were clickable as links before, they should be now.