-
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] Polygon (limited) - Add screen reader experience #2173
Conversation
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (299289f) and published it to npm. You Example: yarn add @khanacademy/perseus@PR2173 If you are working in Khan Academy's webapp, you can run: ./dev/tools/bump_perseus_version.sh -t PR2173 |
Size Change: +1.97 kB (+0.13%) Total Size: 1.48 MB
ℹ️ View Unchanged
|
<UnlimitedPolygonGraph {...statefulProps} /> | ||
) : ( | ||
<LimitedPolygonGraph {...statefulProps} /> | ||
); |
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 changed this to render as components this so I could use hooks inside each component separately. Otherwise it gives me a conditional hook error and crashes in the editor when changing between fixed and unlimited polygon graph type.
I prefer this over having a whole bunch of hooks up here in the parent that may or may not apply to the two different components and passing them down.
srPolygonInteractiveElements: string; | ||
}; | ||
|
||
function describePolygonGraph( |
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 didn't export this one for testing, because I tested all these strings inside the LimitedPolygon component since it's called in there.
export function srFormatNumber( | ||
a: number, | ||
locale: string, | ||
maximumFractionDigits?: number, |
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.
Adding this parameter so that the screen reader label for the side angles can match what's presented by the "show angles" button (which is only one decimal point).
@@ -237,3 +239,103 @@ export function getQuadraticXIntercepts( | |||
|
|||
return [x1, x2]; | |||
} | |||
|
|||
export function getAngleFromPoints(points: Coord[], i: number) { |
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.
Love this! It might be good to add a comment or two just to help save future devs some time.
|
||
// Assert | ||
expect(polygonGraph).toBeInTheDocument(); | ||
expect(polygonGraph).toHaveAccessibleDescription( |
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 love these tests. Very clean and clear about what's being tested.
return angle; | ||
} | ||
|
||
export function radianToDegree(radians: number) { |
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 could have sworn we already had a function for this, possibly in kmath?
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.
Noting here for posterity: kmath had convertDegreesToRadians
but not the other way around. I added convertRadiansToDegrees
in another PR and pulled it in here.
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.
Wow! Another set of amazing improvements - great work!
Summary:
Add the aria labels and descriptions for the full graph and interactive elements in the Fixed Polygon graph, based on the SRUX doc.
The Unlimited Polygon SR experience will be added in a separate PR.
Issue: https://khanacademy.atlassian.net/browse/LEMS-2487
Test plan:
yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/polygon.test.tsx
yarn jest packages/perseus/src/widgets/interactive-graphs/graphs/utils.test.ts
Storybook
http://localhost:6006/iframe.html?globals=&id=perseuseditor-widgets-interactive-graph--interactive-graph-polygon&viewMode=story