fix(LEMS-2225): add conditional aria-describedby logic #2283

anakaren-rojas · 2024-07-26T18:38:54Z

Summary:

Adds conditional logic to allow aria-describedby to be:

defined by the user (overrides default)
available in addition to aria-label or aria-labelledby

Issue: LEMS-2225

Test plan:

yarn start in branch or chromatic link
Navigate to Popover stories
Choose With custom aria described by story
Story includes hidden element that should be read out by the screen reader

Screenshots

Screen.Recording.2024-07-30.at.3.30.44.PM.mov

changeset-bot · 2024-07-26T18:38:57Z

🦋 Changeset detected

Latest commit: c156d96

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-popover	Patch

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 · 2024-07-26T18:39:11Z

Gerald

Required Reviewers

@Khan/wonder-blocks for changes to .changeset/plenty-rockets-breathe.md, __docs__/wonder-blocks-popover/popover.stories.tsx, packages/wonder-blocks-popover/src/components/popover.tsx, packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx

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

github-actions · 2024-07-26T18:39:53Z

npm Snapshot: Published

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

You can install the packages in webapp by running:

./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2283"

Packages can also be installed manually by running:

yarn add @khanacademy/wonder-blocks-<package-name>@PR2283

github-actions · 2024-07-26T18:40:54Z

Size Change: +885 B (+0.94%)

Total Size: 95.3 kB

Filename	Size	Change
`packages/wonder-blocks-form/dist/es/index.js`	5.97 kB	+871 B (+17.08%)	⚠️
`packages/wonder-blocks-popover/dist/es/index.js`	4.83 kB	+8 B (+0.17%)
`packages/wonder-blocks-tooltip/dist/es/index.js`	6.79 kB	+6 B (+0.09%)

ℹ️ View Unchanged

Filename	Size
`packages/wonder-blocks-accordion/dist/es/index.js`	3.78 kB
`packages/wonder-blocks-banner/dist/es/index.js`	1.53 kB
`packages/wonder-blocks-birthday-picker/dist/es/index.js`	1.57 kB
`packages/wonder-blocks-breadcrumbs/dist/es/index.js`	887 B
`packages/wonder-blocks-button/dist/es/index.js`	4.03 kB
`packages/wonder-blocks-cell/dist/es/index.js`	2.01 kB
`packages/wonder-blocks-clickable/dist/es/index.js`	3.06 kB
`packages/wonder-blocks-core/dist/es/index.js`	3.45 kB
`packages/wonder-blocks-data/dist/es/index.js`	6.24 kB
`packages/wonder-blocks-dropdown/dist/es/index.js`	14.1 kB
`packages/wonder-blocks-grid/dist/es/index.js`	1.36 kB
`packages/wonder-blocks-i18n/dist/es/index.js`	4.76 kB
`packages/wonder-blocks-icon-button/dist/es/index.js`	2.99 kB
`packages/wonder-blocks-icon/dist/es/index.js`	828 B
`packages/wonder-blocks-labeled-field/dist/es/index.js`	72 B
`packages/wonder-blocks-layout/dist/es/index.js`	1.82 kB
`packages/wonder-blocks-link/dist/es/index.js`	2.27 kB
`packages/wonder-blocks-modal/dist/es/index.js`	5.36 kB
`packages/wonder-blocks-pill/dist/es/index.js`	1.64 kB
`packages/wonder-blocks-progress-spinner/dist/es/index.js`	1.52 kB
`packages/wonder-blocks-search-field/dist/es/index.js`	1.3 kB
`packages/wonder-blocks-switch/dist/es/index.js`	1.94 kB
`packages/wonder-blocks-testing-core/dist/es/index.js`	3.64 kB
`packages/wonder-blocks-testing/dist/es/index.js`	1.13 kB
`packages/wonder-blocks-theming/dist/es/index.js`	693 B
`packages/wonder-blocks-timing/dist/es/index.js`	1.8 kB
`packages/wonder-blocks-tokens/dist/es/index.js`	1.74 kB
`packages/wonder-blocks-toolbar/dist/es/index.js`	855 B
`packages/wonder-blocks-typography/dist/es/index.js`	1.23 kB

_{compressed-size-action}

github-actions · 2024-07-26T18:41:00Z

A new build was pushed to Chromatic! 🚀

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

Chromatic results:

Metric	Total
Captured snapshots	356
Tests with visual changes	0
Total stories	456
Inherited (not captured) snapshots [TurboSnap]	0
Tests on the build	356

codecov · 2024-07-26T18:42:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.13%. Comparing base (68dd605) to head (fb9e0e0).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2283      +/-   ##
==========================================
+ Coverage   93.83%   95.13%   +1.30%     
==========================================
  Files         252      253       +1     
  Lines       29825    30240     +415     
  Branches     2440     1869     -571     
==========================================
+ Hits        27985    28770     +785     
+ Misses       1793     1463     -330     
+ Partials       47        7      -40

Files	Coverage Δ
...s/wonder-blocks-popover/src/components/popover.tsx	`100.00% <100.00%> (ø)`

... and 47 files with indirect coverage changes

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 68dd605...fb9e0e0. Read the comment docs.

…r-blocks into add-aria-described-by

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

catandthemachines

Just need to clean up one test file and a few comments and I think we're good to go! 🥳

.changeset/plenty-rockets-breathe.md

catandthemachines · 2024-07-31T20:24:51Z

__docs__/wonder-blocks-popover/popover.stories.tsx

@@ -87,6 +87,16 @@ const styles = StyleSheet.create({
        flexDirection: "row",
        gap: spacing.medium_16,
    },
+    srOnly: {


nit: Does WonderBlocks have a utility constant for this? It would be great if it was in a shared file for other WonderBlocks components to use.

It doesn't seem like it; I'd imagine since wonderblocks hosts components, they would all need to be visible

packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx

jandrade

Looks great to me! Just left a minor suggestion on refactoring one of the tests, but nothing blocking. Thanks for working on this improvement

jandrade · 2024-08-01T21:36:01Z

packages/wonder-blocks-popover/src/components/__tests__/popover.test.tsx

+                await screen.findByRole("dialog", {
+                    description: "This is a popover description",


suggestion: With this refactor in place, I think it makes sense to create a separate test to assert the description part of this test.

For some context, we try to follow the AAA approach for testing: https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/98402353/JavaScript+Testing+Best+Practices#Assert-One-Requirement-Per-Test
https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/98402353/JavaScript+Testing+Best+Practices#Assert-One-Requirement-Per-Test

One of the main benefits we have found by using this approach is that it's easier to isolate a failure when it happens and also easier to apply the fix when needed.

Thanks Juan, I split it out into three tests that each have 1 assertion

adding aria described by changes

18163e8

anakaren-rojas self-assigned this Jul 26, 2024

anakaren-rojas marked this pull request as draft July 26, 2024 18:39

khan-actions-bot requested a review from a team July 26, 2024 18:39

catandthemachines and others added 3 commits July 26, 2024 12:27

adding changeset.

1c14000

update logic and story

c54ccaa

Merge branch 'add-aria-described-by' of https://github.com/Khan/wonde…

9913c00

…r-blocks into add-aria-described-by

anakaren-rojas commented Jul 26, 2024

View reviewed changes

packages/wonder-blocks-popover/src/components/popover.tsx Outdated Show resolved Hide resolved

anakaren-rojas added 4 commits July 29, 2024 12:49

remove aria label condition for aria described by

926eed9

update units tests

334dc37

update custom aria described by story

f0a6ec5

lint fix

bebc4a0

anakaren-rojas changed the title ~~adding aria described by changes~~ fix(LEMS-2225): add conditional aria-describedby logic Jul 30, 2024

anakaren-rojas marked this pull request as ready for review July 30, 2024 22:38

anakaren-rojas requested review from handeyeco and catandthemachines July 30, 2024 22:38

catandthemachines approved these changes Jul 31, 2024

View reviewed changes

anakaren-rojas added 3 commits July 31, 2024 13:42

update changeset

14b6b0b

updatet test to be more consistent

e2f0927

update test to include additional check

fb9e0e0

handeyeco requested a review from jandrade August 1, 2024 20:07

jandrade approved these changes Aug 1, 2024

View reviewed changes

update test to split out assertions

c156d96

anakaren-rojas merged commit 166ae1d into main Aug 1, 2024
15 checks passed

anakaren-rojas deleted the add-aria-described-by branch August 1, 2024 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(LEMS-2225): add conditional aria-describedby logic #2283

fix(LEMS-2225): add conditional aria-describedby logic #2283

anakaren-rojas commented Jul 26, 2024 •

edited

Loading

changeset-bot bot commented Jul 26, 2024 •

edited

Loading

khan-actions-bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading

catandthemachines left a comment

catandthemachines Jul 31, 2024

anakaren-rojas Jul 31, 2024

jandrade left a comment

jandrade Aug 1, 2024

anakaren-rojas Aug 1, 2024

		await screen.findByRole("dialog", {
		description: "This is a popover description",

fix(LEMS-2225): add conditional aria-describedby logic #2283

fix(LEMS-2225): add conditional aria-describedby logic #2283

Conversation

anakaren-rojas commented Jul 26, 2024 • edited Loading

Summary:

Test plan:

Screenshots

changeset-bot bot commented Jul 26, 2024 • edited Loading

🦋 Changeset detected

khan-actions-bot commented Jul 26, 2024 • edited Loading

Gerald

github-actions bot commented Jul 26, 2024 • edited Loading

npm Snapshot: Published

github-actions bot commented Jul 26, 2024 • edited Loading

github-actions bot commented Jul 26, 2024 • edited Loading

A new build was pushed to Chromatic! 🚀

Chromatic results:

codecov bot commented Jul 26, 2024 • edited Loading

Codecov Report

catandthemachines left a comment

Choose a reason for hiding this comment

catandthemachines Jul 31, 2024

Choose a reason for hiding this comment

anakaren-rojas Jul 31, 2024

Choose a reason for hiding this comment

jandrade left a comment

Choose a reason for hiding this comment

jandrade Aug 1, 2024

Choose a reason for hiding this comment

anakaren-rojas Aug 1, 2024

Choose a reason for hiding this comment

anakaren-rojas commented Jul 26, 2024 •

edited

Loading

changeset-bot bot commented Jul 26, 2024 •

edited

Loading

khan-actions-bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

github-actions bot commented Jul 26, 2024 •

edited

Loading

codecov bot commented Jul 26, 2024 •

edited

Loading