-
Notifications
You must be signed in to change notification settings - Fork 350
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
feat(LEMS-2526): update aria label to include appearance descriptions #1745
Conversation
Size Change: +245 B (+0.03%) Total Size: 867 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (79844ca) and published it to npm. You Example: yarn add @khanacademy/perseus@PR1745 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR1745 |
GeraldRequired Reviewers
Don't want to be involved in this pull request? 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.
I left some recommendations inline, but I don't see any blocking issues (the code looks correct). LGTM!
packages/perseus-editor/src/widgets/interactive-graph-editor/locked-figures/util.ts
Outdated
Show resolved
Hide resolved
const convertedColor = color === "grayH" ? "gray" : color; | ||
expect(description).toBe(`. Appearance solid ${convertedColor}.`); |
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.
Thanks for adding these thorough tests!
I think using a ternary in the test might be a bit overkill, though — it makes the tests harder to read, and in a way tautological, since now they duplicate the same logic that's in the production code.
An alternative would be to have just two tests: one that exemplifies the "normal" logic of passing the color name through unchanged, and one that calls out the special case of grayH
:
it("describes a solid color", () => {
const description =
generateLockedFigureAppearanceDescription("red");
expect(description).toBe(". Appearance solid red");
});
it("describes grayH as gray", () => {
const description =
generateLockedFigureAppearanceDescription("grayH");
expect(description).toBe(". Appearance solid gray");
});
...erseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-vector-settings.tsx
Outdated
Show resolved
Hide resolved
...rseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-polygon-settings.tsx
Outdated
Show resolved
Hide resolved
...perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-point-settings.tsx
Outdated
Show resolved
Hide resolved
.../perseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-line-settings.tsx
Outdated
Show resolved
Hide resolved
...seus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-function-settings.tsx
Outdated
Show resolved
Hide resolved
...rseus-editor/src/widgets/interactive-graph-editor/locked-figures/locked-ellipse-settings.tsx
Outdated
Show resolved
Hide resolved
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 great to me!
Summary:
Updates the aria description to include visual traits
New aria label structure will be:
Element Type
Visible Label(s)
Math Details
Visual Traits
Note:
Issue: LEMS-2526
Test plan:
Expected Behavior
Visual Traits
of each of the locked figures should change when an input (color, line style, fill) is updated and the generate aria label button is pressedScreen recording
Screen.Recording.2024-10-17.at.11.13.27.AM.mov