Card: updates labeling and internal testId support #2859

marcysutton · 2025-11-12T22:09:39Z

Summary:

When integrating Cards into the frontend repo, I ran into a few complications that I wanted to fix:

The labeling requirements made it difficult to create wrapped Cards
The labeling was also required for scenarios where it doesn't need to be -- not all sections or figures need accessible names. So I made these optional
The dismiss button was missing a testId or way to create one, so existing tests failed

Issue: FEI-6310

Related frontend PR: https://github.com/Khan/frontend/pull/5283

Test plan:

Review typing to ensure it makes sense
Ensure tests pass

changeset-bot · 2025-11-12T22:09:44Z

🦋 Changeset detected

Latest commit: 731cada

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@khanacademy/wonder-blocks-card	Minor

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

khan-actions-bot · 2025-11-12T22:09:53Z

Gerald

Required Reviewers

@Khan/wonder-blocks for changes to .changeset/thin-plants-decide.md, __docs__/wonder-blocks-card/accessibility.mdx, packages/wonder-blocks-card/src/components/card.tsx, packages/wonder-blocks-card/src/components/dismiss-button.tsx, packages/wonder-blocks-card/src/__tests__/components/card.test.tsx, packages/wonder-blocks-card/src/__tests__/components/card.typestest.tsx

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2025-11-12T22:10:46Z

Size Change: +54 B (+0.05%)

Total Size: 109 kB

Filename	Size	Change
`packages/wonder-blocks-card/dist/es/index.js`	1.06 kB	+54 B (+5.35%)	🔍

ℹ️ View Unchanged

Filename	Size
`packages/wonder-blocks-accordion/dist/es/index.js`	3 kB
`packages/wonder-blocks-announcer/dist/es/index.js`	1.74 kB
`packages/wonder-blocks-badge/dist/es/index.js`	2.02 kB
`packages/wonder-blocks-banner/dist/es/index.js`	2.01 kB
`packages/wonder-blocks-birthday-picker/dist/es/index.js`	1.92 kB
`packages/wonder-blocks-breadcrumbs/dist/es/index.js`	755 B
`packages/wonder-blocks-button/dist/es/index.js`	4.25 kB
`packages/wonder-blocks-cell/dist/es/index.js`	2.19 kB
`packages/wonder-blocks-clickable/dist/es/index.js`	2.66 kB
`packages/wonder-blocks-core/dist/es/index.js`	2.48 kB
`packages/wonder-blocks-data/dist/es/index.js`	5.48 kB
`packages/wonder-blocks-dropdown/dist/es/index.js`	19.4 kB
`packages/wonder-blocks-form/dist/es/index.js`	6.2 kB
`packages/wonder-blocks-grid/dist/es/index.js`	1.24 kB
`packages/wonder-blocks-icon-button/dist/es/index.js`	3.16 kB
`packages/wonder-blocks-icon/dist/es/index.js`	1.91 kB
`packages/wonder-blocks-labeled-field/dist/es/index.js`	3.48 kB
`packages/wonder-blocks-layout/dist/es/index.js`	1.63 kB
`packages/wonder-blocks-link/dist/es/index.js`	1.52 kB
`packages/wonder-blocks-modal/dist/es/index.js`	7.06 kB
`packages/wonder-blocks-pill/dist/es/index.js`	1.31 kB
`packages/wonder-blocks-popover/dist/es/index.js`	4.3 kB
`packages/wonder-blocks-progress-spinner/dist/es/index.js`	1.48 kB
`packages/wonder-blocks-search-field/dist/es/index.js`	1.1 kB
`packages/wonder-blocks-styles/dist/es/index.js`	464 B
`packages/wonder-blocks-switch/dist/es/index.js`	1.55 kB
`packages/wonder-blocks-tabs/dist/es/index.js`	3.71 kB
`packages/wonder-blocks-testing-core/dist/es/index.js`	3.25 kB
`packages/wonder-blocks-testing/dist/es/index.js`	978 B
`packages/wonder-blocks-theming/dist/es/index.js`	384 B
`packages/wonder-blocks-timing/dist/es/index.js`	1.37 kB
`packages/wonder-blocks-tokens/dist/es/index.js`	5.01 kB
`packages/wonder-blocks-toolbar/dist/es/index.js`	906 B
`packages/wonder-blocks-tooltip/dist/es/index.js`	6.4 kB
`packages/wonder-blocks-typography/dist/es/index.js`	1.57 kB

_{compressed-size-action}

github-actions · 2025-11-12T22:10:52Z

npm Snapshot: Published

🎉 Good news!! We've packaged up the latest commit from this PR (6d4735f) and published all packages with changesets to npm.

You can install the packages in frontend by running:

./dev/tools/deploy_wonder_blocks.js --tag="PR2859"

Packages can also be installed manually by running:

pnpm add @khanacademy/wonder-blocks-<package-name>@PR2859

github-actions · 2025-11-12T22:11:48Z

A new build was pushed to Chromatic! 🚀

https://5e1bf4b385e3fb0020b7073c-qgtrabsocj.chromatic.com/

Chromatic results:

Metric	Total
Captured snapshots	376
Tests with visual changes	0
Total stories	741
Inherited (not captured) snapshots [TurboSnap]	253
Tests on the build	441

marcysutton · 2025-11-13T18:40:03Z

I think this one is finally ready to go -- it is supporting a PR in the frontend repo through a snapshot release: https://github.com/Khan/frontend/pull/5283

jandrade · 2025-11-14T15:48:38Z

packages/wonder-blocks-card/src/components/card.tsx

+    // 1. labels.cardAriaLabel (preferred for translatable strings)
+    // 2. aria-labelledby (if provided, don't set aria-label)
+    // 3. aria-label (fallback)


thought: Based on this description, it looks like cardAriaLabel and aria-label have the same function, so one of these would be redundant, as aria-label would technically be translatable as well.

Yes -- and aria-label is likely translated on the way in, anyway. Originally the API for this component was to only use labels.cardAriaLabel and apply that internally to aria-label. But I wanted to make it more flexible based on existing usage where aria-label was passed down through multiple layers of abstraction as a prop.

The "only one label type" and specific tag restrictions made the Card super hard to use in practice, so I loosened those types to make it more reasonable.

Maybe I'll just remove this comment entirely.

For ease of use, I think it would be easier to have only one way consumers can set the aria-label.

Since aria-label and labels.cardAriaLabel are for the same element, could we use only one of them? I prefer the aria-label prop over the labels.cardAriaLabel prop since it is consistent with the other WB components! (I might be misunderstanding the context around needing both, so let me know!)

I originally had aria-label in the Card Implementation Spec but labels were suggested instead so I went with that... The labels structure also includes dismissButtonAriaLabel, which is convenient.

I decided to allow aria-label in the integration phase in frontend, based on how the Card component was wrapped to create deeper abstractions. We could go with all aria-label, but the dismissButtonAriaLabel will be an odd one out in the labels object or have another one-off prop.

I'm opting to keep labels only and getting rid of aria-label, since there are two properties on the object! (whenever this Git outage is resolved)

Ah right, it's been a while since I took a look at the Card Implementation Spec! I think keeping it on labels works too, so there's only 1 way to add the aria-label! Thanks Marcy!

beaesguerra

Thanks for the improvements! Left some comments/questions on things I'd like to get your thoughts on!

beaesguerra · 2025-11-17T21:12:42Z

packages/wonder-blocks-card/src/components/card.tsx

+    "aria-labelledby"?: string;
+    "aria-label"?: string;
+    "aria-busy"?: AriaAttributes["aria-busy"];
+    "aria-roledescription"?: AriaAttributes["aria-roledescription"];


Should the Card component props extend the AriaProps? This is normally what we do in components so it supports all the aria attributes!

Also, if we want to include support for setting the role, AriaProps includes the type for role, while AriaAttributes doesn't!

I personally don't think it's a good practice to support all ARIA attributes everywhere because it can allow non-compliant markup. We need a way to restrict some of them to valid use cases, so opting in for common usage through direct props makes sense to me. We can always enable support for something specific if and when it comes up!

That's fair! I think this makes sense to start doing going forwards. cc: @jandrade also in case you have thoughts on this!

Another nice thing about this is when we explicitly add the specific aria props, it'll get included in the prop docs automatically! (props extended from the AriaProps weren't automatically included the props docs)

beaesguerra · 2025-11-17T21:16:45Z

packages/wonder-blocks-card/src/components/card.tsx

        labels,
        tag,
-        testId,
+        testId = "card",


Could we leave testId uninitialized and only set the test id if it is provided? That way there aren't unnecessary data attributes in the DOM, and teams will have to explicitly pass in a test id if they want to use it! This is the behaviour I've observed in other components!

We can also only set the close button test id if the main testId prop is provided! What do you think?

beaesguerra · 2025-11-17T21:23:27Z

packages/wonder-blocks-card/src/components/card.tsx

+    // 1. labels.cardAriaLabel (preferred for translatable strings)
+    // 2. aria-labelledby (if provided, don't set aria-label)
+    // 3. aria-label (fallback)


For ease of use, I think it would be easier to have only one way consumers can set the aria-label.

Since aria-label and labels.cardAriaLabel are for the same element, could we use only one of them? I prefer the aria-label prop over the labels.cardAriaLabel prop since it is consistent with the other WB components! (I might be misunderstanding the context around needing both, so let me know!)

… adds testId to dismiss button

beaesguerra · 2025-11-19T00:18:38Z

packages/wonder-blocks-card/src/components/card.tsx

+        : ariaLabelledBy
+          ? undefined
+          : undefined;


Could the ariaLabelledBy ? undefined : undefined be simplified to undefined? 😄

beaesguerra · 2025-11-19T00:22:41Z

packages/wonder-blocks-card/src/components/card.tsx

+            aria-label={ariaLabelValue}
+            aria-labelledby={ariaLabelledBy}


Based on the prop docs:

aria-labelledby - For ID references (will take precedence over aria-label if both are provided).

There's a case where both the aria-label and aria-labelledby attributes can be set! Does aria-label need to be undefined if aria-labelledby is set?

I was trying to simplify the typing since it got so complicated with wrapper components... I updated this to be mutually exclusive again, here's hoping it didn't reintroduce nightmare typing for consumers! (discriminated unions are so cryptic!) 🙏

beaesguerra · 2025-11-19T00:23:43Z

packages/wonder-blocks-card/src/components/card.tsx

                <DismissButton
                    aria-label={labels?.dismissButtonAriaLabel || "Close"}
                    onClick={(e) => onDismiss?.(e)}
+                    testId={`${testId}-dismiss-button`}


Suggested change

testId={`${testId}-dismiss-button`}

testId={testId && `${testId}-dismiss-button`}

We can check for testId first to avoid having a undefined-dismiss-button test id all the time on the close button!

Added a fix for this!

beaesguerra

Looks good to me! 🚀

codecov · 2025-11-19T19:06:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (415dd5a) to head (731cada).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@     Coverage Diff      @@
##   main   #2859   +/-   ##
============================
============================

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 415dd5a...731cada. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

marcysutton self-assigned this Nov 12, 2025

marcysutton added the olc-5.0.d5056 label Nov 12, 2025

khan-actions-bot requested a review from a team November 12, 2025 22:09

jandrade reviewed Nov 14, 2025

View reviewed changes

khan-actions-bot requested a review from a team November 14, 2025 17:45

beaesguerra reviewed Nov 17, 2025

View reviewed changes

khan-actions-bot requested a review from a team November 18, 2025 20:30

marcysutton force-pushed the card-integration branch from 387b0b5 to e63bfae Compare November 18, 2025 22:51

marcysutton added 9 commits November 18, 2025 14:55

[card-integration] Add testId to dismiss button

22cac47

[card-integration] Revisit Card typing

6684e9e

[card-integration] docs(changeset): Updates labeling requirements and…

6bab6f5

… adds testId to dismiss button

[card-integration] Simplify typing

86884e1

[card-integration] Allow aria-busy, aria-roledescription

caf4de5

[card-integration] Update select Aria attr types

dc6be33

[card-integration] Update comments

c346778

[card-integration] PR feedback

1eec27e

[card-integration] Update naming rules

82d49c0

marcysutton force-pushed the card-integration branch from e63bfae to 82d49c0 Compare November 18, 2025 22:55

beaesguerra reviewed Nov 19, 2025

View reviewed changes

[card-integration] Update labeling

731cada

khan-actions-bot requested a review from a team November 19, 2025 00:44

beaesguerra approved these changes Nov 19, 2025

View reviewed changes

marcysutton merged commit d36c492 into main Nov 19, 2025
15 checks passed

marcysutton deleted the card-integration branch November 19, 2025 19:05

	testId={`${testId}-dismiss-button`}
	testId={testId && `${testId}-dismiss-button`}

Card: updates labeling and internal testId support #2859

Card: updates labeling and internal testId support #2859

Uh oh!

Conversation

marcysutton commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

Test plan:

Uh oh!

changeset-bot bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

khan-actions-bot commented Nov 12, 2025

Gerald

Uh oh!

github-actions bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

npm Snapshot: Published

Uh oh!

github-actions bot commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

A new build was pushed to Chromatic! 🚀

Chromatic results:

Uh oh!

marcysutton commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcysutton Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beaesguerra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

beaesguerra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Nov 19, 2025

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

marcysutton commented Nov 12, 2025 •

edited

Loading

changeset-bot bot commented Nov 12, 2025 •

edited

Loading

github-actions bot commented Nov 12, 2025 •

edited

Loading

github-actions bot commented Nov 12, 2025 •

edited

Loading

github-actions bot commented Nov 12, 2025 •

edited

Loading

marcysutton Nov 14, 2025 •

edited

Loading