-
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
[SR] Quadratic - add screen reader support for Quadratic interactive graph #2136
base: main
Are you sure you want to change the base?
Conversation
The [SRUX doc](https://khanacademy.atlassian.net/wiki/spaces/LC/pages/3460366348/Linear) still needs a label for the grab handle, but I tried my best in the meantime. - Add a label and describedby for the grab handle. - Add aria-live states for the different interactive elements so they don't override each other. Issue: https://khanacademy.atlassian.net/browse/LEMS-1726 Test plan: `yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/linear.test.tsx` Storybook - http://localhost:6006/iframe.html?id=perseuseditor-widgets-interactive-graph--interactive-graph-linear&viewMode=story - Try all the different slopes and intercepts - Move different elements and confirm that the updated aria-label is what is read out, and none of the other elements override the currently focused one.
…inear System interactive graph
…atic interactive graph
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.
Renamed to quadratic.test.tsx
since it has React components now.
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (bd82b4c) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2136 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2136 |
Size Change: +1.78 kB (+0.12%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
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 don't have a lot of the context for the holistic approach to screenreader strings in Interactive graph, but gosh if this isn't just a gorgeous PR.
Brilliant work, and what a great improvement for screen readers!
${"up, quadrant vertex, 2 intercepts"} | ${[[-6, 5], [-1, -1], [4, 5]]} | ${"upward"} | ${"in quadrant 3"} | ${[1.041, -3.041]} | ${-0.76} | ||
${"sloped line, no vertex, 1 intercept"} | ${[[-5, -5], [0, 0], [5, 5]]} | ${undefined} | ${undefined} | ${[0]} | ${0} | ||
${"horizontal line, no vertex, no intercept"} | ${[[-5, 1], [0, 1], [5, 1]]} | ${undefined} | ${undefined} | ${[]} | ${1} | ||
`( |
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.
This is beautiful. :)
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.
The SR experience is just beautiful, and so many great tests and improvements to the code structure. Ship it! 🥳
@@ -17,10 +27,14 @@ import type {QuadraticCoefficient, QuadraticCoords} from "@khanacademy/kmath"; | |||
export function renderQuadraticGraph( | |||
state: QuadraticGraphState, | |||
dispatch: Dispatch, | |||
i18n: I18nContextType, |
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.
Yay!! Thank you for fixing this. 🥰
strings, | ||
locale, | ||
); | ||
// Vertex might not exist if the quadratic graph is a line |
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.
Nice catch with this.
): string { | ||
const strings = describeQuadraticGraph(state, i18n); | ||
// This particular string will never be undefined. | ||
return strings.srQuadraticInteractiveElements || ""; |
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.
If this string will always be defined, could you update the describeQuadraticGraph
to return type Record<string, string>
, not Record<string, string | undefined>
Summary:
Add the aria label and descriptions for the full graph and interactive elements
in the Quadratic graph, based on the SRUX doc.
NOTE: The SRUX doc doesn't yet cover some edge cases, so I handled those to the
best of my ability.
Issue: https://khanacademy.atlassian.net/browse/LEMS-2486
Test plan:
yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/quadratic.test.tsx
Storybook