Skip to content
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] Circle - Add the interactive elements circle description to the whole graph container #2060

Merged
merged 4 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/little-beds-tickle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

[SR] Circle - Add interactive Circle element to full graph description
2 changes: 1 addition & 1 deletion packages/perseus/src/components/i18n-context.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {mockStrings} from "../strings";

import type {PerseusStrings} from "../strings";

type I18nContextType = {
export type I18nContextType = {
strings: PerseusStrings;
locale: string;
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,12 @@ import * as React from "react";
import {Dependencies} from "@khanacademy/perseus";

import {testDependencies} from "../../../../../../testing/test-dependencies";
import {mockPerseusI18nContext} from "../../../components/i18n-context";
import {MafsGraph} from "../mafs-graph";
import {getBaseMafsGraphPropsForTests} from "../utils";

import {describeCircleGraph} from "./circle";

import type {InteractiveGraphState} from "../types";
import type {UserEvent} from "@testing-library/user-event";

Expand Down Expand Up @@ -163,3 +166,57 @@ describe("Circle graph screen reader", () => {
expect(radiusPoint).toHaveAttribute("aria-live", "off");
});
});

describe("describeCircleGraph", () => {
test("describes a default circle", () => {
// Arrange

// Act
const strings = describeCircleGraph(
baseCircleState,
mockPerseusI18nContext,
);

// Assert
expect(strings.srCircleGraph).toBe("A circle on a coordinate plane.");
expect(strings.srCircleShape).toBe(
"Circle. The center point is at 0 comma 0.",
);
expect(strings.srCircleRadiusPoint).toBe("Radius point at 1 comma 0.");
expect(strings.srCircleRadius).toBe("Circle radius is 1.");
expect(strings.srCircleOuterPoints).toBe(
"Points on the circle at 1 comma 0, 0 comma 1, -1 comma 0, 0 comma -1.",
);
expect(strings.srCircleInteractiveElement).toBe(
"Interactive elements: Circle. The center point is at 0 comma 0. Circle radius is 1.",
);
});

test("describes a circle with updated values", () => {
// Arrange

// Act
const strings = describeCircleGraph(
{
...baseCircleState,
center: [2, 3],
radiusPoint: [7, 3],
},
mockPerseusI18nContext,
);

// Assert
expect(strings.srCircleGraph).toBe("A circle on a coordinate plane.");
expect(strings.srCircleShape).toBe(
"Circle. The center point is at 2 comma 3.",
);
expect(strings.srCircleRadiusPoint).toBe("Radius point at 7 comma 3.");
expect(strings.srCircleRadius).toBe("Circle radius is 5.");
expect(strings.srCircleOuterPoints).toBe(
"Points on the circle at 7 comma 3, 2 comma 8, -3 comma 3, 2 comma -2.",
);
expect(strings.srCircleInteractiveElement).toBe(
"Interactive elements: Circle. The center point is at 2 comma 3. Circle radius is 5.",
);
});
});
99 changes: 71 additions & 28 deletions packages/perseus/src/widgets/interactive-graphs/graphs/circle.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
useTransformVectorsToPixels,
} from "./use-transform";

import type {I18nContextType} from "../../../components/i18n-context";
import type {
AriaLive,
CircleGraphState,
Expand All @@ -30,7 +31,7 @@ export function renderCircleGraph(
): InteractiveGraphElementSuite {
return {
graph: <CircleGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: null,
interactiveElementsDescription: CircleGraphDescription({state}),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly surprised that this works. Since CircleGraphDescription is calling a hook, I'd expect it to have to be used as a JSX element:

<CircleGraphDescription state={state} />

If the component calling renderCircleGraph does so conditionally, e.g.

if (graphType === "circle") {
  return renderCircleGraph(state, dispatch)
}

And graphType changes between renders, this code will break the Rules of Hooks. The usePerseusI18n hook will be called on one render and not called on the next.

I think it's working (mostly?) because the graph type rarely changes between renders. It probably only does so in the content editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack! My mistake. I 100% meant to use your code for Point as the blueprint for this, which means I was supposed to write <CircleGraphDescription state={state} />.

};
}

Expand All @@ -51,40 +52,25 @@ function CircleGraph(props: CircleGraphProps) {
const outerPointsId = id + "-outer-points";

// Aria label strings
const circleGraphAriaLabel = strings.srCircleGraph;
const circleShapeAriaLabel = strings.srCircleShape({
centerX: srFormatNumber(center[0], locale),
centerY: srFormatNumber(center[1], locale),
});
const circleRadiusPointAriaLabel = strings.srCircleRadiusPoint({
radiusPointX: srFormatNumber(radiusPoint[0], locale),
radiusPointY: srFormatNumber(radiusPoint[1], locale),
});
const circleRadiusDescription = strings.srCircleRadius({
radius,
});
const circleOuterPointsDescription = strings.srCircleOuterPoints({
point1X: srFormatNumber(center[0] + radius, locale),
point1Y: srFormatNumber(center[1], locale),
point2X: srFormatNumber(center[0], locale),
point2Y: srFormatNumber(center[1] + radius, locale),
point3X: srFormatNumber(center[0] - radius, locale),
point3Y: srFormatNumber(center[1], locale),
point4X: srFormatNumber(center[0], locale),
point4Y: srFormatNumber(center[1] - radius, locale),
});
const {
srCircleGraph,
srCircleShape,
srCircleRadiusPoint,
srCircleRadius,
srCircleOuterPoints,
} = describeCircleGraph(graphState, {strings, locale});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this way of splitting up the logic! Makes it much easier to read and test :)


return (
<g
// Outer circle minimal description
aria-label={circleGraphAriaLabel}
aria-label={srCircleGraph}
aria-describedby={`${circleId} ${radiusId} ${outerPointsId}`}
>
<MovableCircle
id={circleId}
// Focusable circle aria label reads with every update
// because of the aria-live property in the circle <g>.
ariaLabel={circleShapeAriaLabel}
ariaLabel={srCircleShape}
// Aria-describedby describes additional info on focus.
ariaDescribedBy={`${radiusId} ${outerPointsId}`}
center={center}
Expand All @@ -96,7 +82,7 @@ function CircleGraph(props: CircleGraphProps) {
/>
<MovablePoint
// Radius point aria label reads with every update.
ariaLabel={`${circleRadiusPointAriaLabel} ${circleRadiusDescription}`}
ariaLabel={`${srCircleRadiusPoint} ${srCircleRadius}`}
// Aria-describedby describes additional info on focus.
ariaDescribedBy={`${outerPointsId}`}
// The radius point's aria-live property is set to "off" when
Expand All @@ -116,10 +102,10 @@ function CircleGraph(props: CircleGraphProps) {
{/* Hidden elements to provide the descriptions for the
circle and radius point's `aria-describedby` properties. */}
<g id={radiusId} style={{display: "hidden"}}>
{circleRadiusDescription}
{srCircleRadius}
</g>
<g id={outerPointsId} style={{display: "hidden"}}>
{circleOuterPointsDescription}
{srCircleOuterPoints}
</g>
</g>
);
Expand Down Expand Up @@ -221,3 +207,60 @@ function crossProduct<A, B>(as: A[], bs: B[]): [A, B][] {
}
return result;
}

function CircleGraphDescription({state}: {state: CircleGraphState}) {
Copy link
Contributor

@anakaren-rojas anakaren-rojas Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the same as state: CircleGraphState? Or is there an additional benefit to the destructing in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that works too. I think the reason it was done this way (I was following what Ben did for the point description) is so it follows the prop structure the way they're standardly written for other functional components.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// The reason that CircleGraphDescription is a component (rather than a
// function that returns a string) is because it needs to use a
// hook: `usePerseusI18n`.
const i18n = usePerseusI18n();
const strings = describeCircleGraph(state, i18n);

return strings.srCircleInteractiveElement;
}

// Exported for testing
export function describeCircleGraph(
state: CircleGraphState,
i18n: I18nContextType,
): Record<string, string> {
const {strings, locale} = i18n;
const {center, radiusPoint} = state;
const radius = getRadius(state);

// Aria label strings
const srCircleGraph = strings.srCircleGraph;
const srCircleShape = strings.srCircleShape({
centerX: srFormatNumber(center[0], locale),
centerY: srFormatNumber(center[1], locale),
});
const srCircleRadiusPoint = strings.srCircleRadiusPoint({
radiusPointX: srFormatNumber(radiusPoint[0], locale),
radiusPointY: srFormatNumber(radiusPoint[1], locale),
});
const srCircleRadius = strings.srCircleRadius({
radius,
});
const srCircleOuterPoints = strings.srCircleOuterPoints({
point1X: srFormatNumber(center[0] + radius, locale),
point1Y: srFormatNumber(center[1], locale),
point2X: srFormatNumber(center[0], locale),
point2Y: srFormatNumber(center[1] + radius, locale),
point3X: srFormatNumber(center[0] - radius, locale),
point3Y: srFormatNumber(center[1], locale),
point4X: srFormatNumber(center[0], locale),
point4Y: srFormatNumber(center[1] - radius, locale),
});

const srCircleInteractiveElement = strings.srInteractiveElements({
elements: [srCircleShape, srCircleRadius].join(" "),
});

return {
srCircleGraph,
srCircleShape,
srCircleRadiusPoint,
srCircleRadius,
srCircleOuterPoints,
srCircleInteractiveElement,
};
}
Loading