Card: address accessibility review feedback#7852
Conversation
🦋 Changeset detectedLatest commit: 45e4ed7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
…react into liuliu/card-improvements
There was a problem hiding this comment.
Pull request overview
Updates the experimental Card component to address external accessibility review feedback by tightening runtime/dev-time validation, improving a11y-focused examples and docs, and aligning with the repo’s data-component instrumentation approach.
Changes:
- Added
data-componentattributes acrossCardand subcomponents; added anasprop to support rendering as a labelled<section>landmark (with runtime dev warnings for missing accessible name). - Made
childrenrequired and added a dev warning +nullrender for empty cards, with new unit test coverage. - Improved Storybook examples and docs (new feature stories, removed inline styles in stories, expanded docs.json props coverage).
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Card/Card.tsx | Adds children requirement, as="section" support + a11y warnings, and data-component attributes for Card + subcomponents. |
| packages/react/src/Card/Card.test.tsx | Adds tests for data-component, empty-children behavior, and as="section" landmark behavior/warnings. |
| packages/react/src/Card/Card.stories.tsx | Updates default/playground stories; uses CSS module decorator instead of inline max-width styles; updates metadata example content. |
| packages/react/src/Card/Card.stories.module.css | Introduces shared Storybook layout classes (width constraint, list layout, custom content layout). |
| packages/react/src/Card/Card.features.stories.tsx | Adds feature stories demonstrating menu usage, landmark section usage, list usage, and interactive content a11y labeling patterns. |
| packages/react/src/Card/Card.docs.json | Documents children requirement, as prop guidance, and props for Card.Description, Card.Menu, and Card.Metadata; adds new stories to docs. |
| .changeset/lucky-terms-sing.md | Declares a minor release for the new props/requirements and instrumentation updates. |
Copilot's findings
- Files reviewed: 7/16 changed files
- Comments generated: 2
joshblack
left a comment
There was a problem hiding this comment.
Looking great! Just left a couple of comments/questions, hope they make sense. Let me know if you want to talk about anything 👀
| warning( | ||
| isEmpty, | ||
| 'The <Card> component was rendered with no children and will not render. Provide either Card subcomponents (Card.Heading, Card.Description, etc.) or custom content.', | ||
| ) | ||
|
|
||
| warning( | ||
| as === 'section' && !('aria-label' in props) && !('aria-labelledby' in props), | ||
| 'The <Card> component used with `as="section"` requires either `aria-label` or `aria-labelledby` so screen-reader users can identify the labelled region. Typically `aria-labelledby` should reference the id of the `Card.Heading`.', | ||
| ) | ||
|
|
||
| if (isEmpty) { | ||
| return null | ||
| } |
There was a problem hiding this comment.
I think both of these are either already enforced at the type level or could be enforced at the type level, is that right? If so, I don't know if we need to do runtime checks for them. Let me know what you think 👀
There was a problem hiding this comment.
Removed the section aria-label warning.
Kept the empty children warning, seems that types can't prevent <Card>{null}</Card> 👀
There was a problem hiding this comment.
@liuliu-dev is this something that we need to warn about? (having no children).
There was a problem hiding this comment.
Nope, removed it
| ) | ||
| } | ||
|
|
||
| CardImage.displayName = 'Card.Image' |
There was a problem hiding this comment.
Super nitpicky, it'd be nice if the components were function declarations so that we didn't have to do this, e.g.
function CardImage(/* ... */) {
// ...
}There was a problem hiding this comment.
Converted to function declarations.
Should we keep displayName for the dotted names? Otherwise it will show as CardImage not Card.Image in devtools
There was a problem hiding this comment.
I'm neutral about it, if I had to pick I'm happy with just the component name itself versus adding the dots in personally but I get if that's confusing
| <Card.Description> | ||
| {"GitHub's design system implemented as React components for building consistent user interfaces."} | ||
| </Card.Description> | ||
| <Card.Menu> |
There was a problem hiding this comment.
One naming suggestion, would it make sense to have Card.Menu be Card.Actions? Or is it important for it to always be a Menu?
| <Card.Menu> | |
| <Card.Actions> |
There was a problem hiding this comment.
I think "Actions" might imply buttons rather than a single dropdown trigger. "Menu" is accurate for the current use case but too specific. What do you think about Card.Control?
There was a problem hiding this comment.
I think if the idea is that this will always be a Menu then that makes sense 👍 If it might grow in the future it might make sense to make it more generic. For cards, do you think we're in the more specific case? 👀
There was a problem hiding this comment.
That makes sense, renamed it to Card.Action.
|
@joshblack thanks for the thorough review, really appreciate it!! I addressed most of them, and some I left questions or marked as features to add later, what do you think? |
joshblack
left a comment
There was a problem hiding this comment.
Approving in advance ✅ Left a couple of questions, only big thing is that I don't think we need to warn on empty children but that's definitely a personal preference.
…react into liuliu/card-improvements
|
🤖 Lint issues have been automatically fixed and committed to this PR. |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/21334 |
Addresses accessibility-review feedback from Tetralogical (github/accessibility#10648) for the experimental
Cardcomponent. Part of https://github.com/github/primer/issues/6672Changelog
Default example - metadata content
Default story no longer uses "Updated 2 hours ago" (which would have pulled in
RelativeTime's outstanding accessibility issues). Replaced with<PeopleIcon /> 3 contributors. The metadata slot is also now documented inCard.docs.jsonand JSDoc to confirm it accepts any content, including other components.Data-component attributes
Added
data-componentattributes per ADR-023 toCard,Card.Icon,Card.Image,Card.Heading,Card.Description,Card.Metadata, andCard.Menu.Grouping content for accessibility — new
aspropCards can now opt into being a
<section>landmark via a newasprop. The prop is typed soas="section"requiresaria-labeloraria-labelledby.Interactive content
New
WithMenufeature story showing an action menu inside a Card with an accessible name on the trigger (aria-label="More options for primer/react").Custom content variant
CustomContentstory now uses an<h3>heading instead of<strong>, resolving the WCAG 2.2 SC 1.3.1 violation.Props section
Card.docs.jsonand JSDoc now documentCard.Description,Card.Menu, andCard.Metadata.Empty props
childrenis now required onCard. Empty cards log a dev warning and returnnull. Tests cover both paths.Inline style attributes on wrapping divs
Added
Card.stories.module.csswith aWidthConstraintContainerclass instead ofstyle={{…}}props in the stories.Rollout strategy
Testing & Reviewing
Merge checklist