Skip to content

Commit

Permalink
[SR] Refactor - Update graphs to pass in i18n context (#2201)
Browse files Browse the repository at this point in the history
## Summary:
When getting the graphs' description strings via `renderGraphElements` in `mafs-graph.tsx`,
we're getting a component with a string with each component separately running the
Perseus i18n hook.

Instead, we can just gave `renderGraphElements` return a string, and we can pass down the
i18n info from the parent where the hook is called once.

This should be easier for people to follow when looking at the code in the future.

Issue: https://khanacademy.atlassian.net/browse/LEMS-2800

## Test plan:
`yarn jest` to run the specific graph tests (`[graphType].test.tsx`)

Storybook
- https://khan.github.io/perseus/?path=/docs/perseuseditor-widgets-interactive-graph--docs
- Check each graph's SR tree to confirm that the "Interactive elements: ..." text
  is still correct.

Author: nishasy

Reviewers: nishasy, catandthemachines

Required Reviewers:

Approved By: catandthemachines

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2201
  • Loading branch information
nishasy authored Feb 13, 2025
1 parent 8e4cb7f commit 91cede4
Show file tree
Hide file tree
Showing 11 changed files with 54 additions and 84 deletions.
5 changes: 5 additions & 0 deletions .changeset/calm-cats-ring.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

[SR] Refactor - Update graphs to pass in i18n context
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ import type {
export function renderCircleGraph(
state: CircleGraphState,
dispatch: Dispatch,
i18n: I18nContextType,
): InteractiveGraphElementSuite {
return {
graph: <CircleGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: (
<CircleGraphDescription state={state} />
),
interactiveElementsDescription: getCircleGraphDescription(state, i18n),
};
}

Expand Down Expand Up @@ -211,13 +210,11 @@ function crossProduct<A, B>(as: A[], bs: B[]): [A, B][] {
return result;
}

function CircleGraphDescription({state}: {state: CircleGraphState}) {
// 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();
function getCircleGraphDescription(
state: CircleGraphState,
i18n: I18nContextType,
) {
const strings = describeCircleGraph(state, i18n);

return strings.srCircleInteractiveElement;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {mockPerseusI18nContext} from "../../../components/i18n-context";
import {MafsGraph} from "../mafs-graph";
import {getBaseMafsGraphPropsForTests} from "../utils";

import {describeLinearSystemGraph} from "./linear-system";
import {getLinearSystemGraphDescription} from "./linear-system";

import type {InteractiveGraphState} from "../types";
import type {UserEvent} from "@testing-library/user-event";
Expand Down Expand Up @@ -256,12 +256,12 @@ describe("Linear System graph screen reader", () => {
});
});

describe(describeLinearSystemGraph, () => {
describe("getLinearSystemGraphDescription", () => {
test("describes a default linear system graph", () => {
// Arrange

// Act
const linearSystemGraphDescription = describeLinearSystemGraph(
const linearSystemGraphDescription = getLinearSystemGraphDescription(
baseLinearSystemState,
mockPerseusI18nContext,
);
Expand All @@ -276,7 +276,7 @@ describe(describeLinearSystemGraph, () => {
// Arrange

// Act
const linearSystemGraphDescription = describeLinearSystemGraph(
const linearSystemGraphDescription = getLinearSystemGraphDescription(
{
...baseLinearSystemState,
coords: [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,13 @@ import type {vec} from "mafs";
export function renderLinearSystemGraph(
state: LinearSystemGraphState,
dispatch: Dispatch,
i18n: I18nContextType,
): InteractiveGraphElementSuite {
return {
graph: <LinearSystemGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: (
<LinearSystemGraphDescription state={state} />
interactiveElementsDescription: getLinearSystemGraphDescription(
state,
i18n,
),
};
}
Expand Down Expand Up @@ -153,21 +155,8 @@ const LinearSystemGraph = (props: LinearSystemGraphProps) => {
);
};

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

return describeLinearSystemGraph(state, i18n);
}

// Exported for testing
export function describeLinearSystemGraph(
export function getLinearSystemGraphDescription(
state: LinearSystemGraphState,
i18n: I18nContextType,
): string {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,11 @@ import type {vec} from "mafs";
export function renderLinearGraph(
state: LinearGraphState,
dispatch: Dispatch,
i18n: I18nContextType,
): InteractiveGraphElementSuite {
return {
graph: <LinearGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: (
<LinearGraphDescription state={state} />
),
interactiveElementsDescription: getLinearGraphDescription(state, i18n),
};
}

Expand Down Expand Up @@ -96,11 +95,10 @@ const LinearGraph = (props: LinearGraphProps, key: number) => {
);
};

function LinearGraphDescription({state}: {state: LinearGraphState}) {
// The reason that LinearGraphDescription is a component (rather than a
// function that returns a string) is because it needs to use a
// hook: `usePerseusI18n`.
const i18n = usePerseusI18n();
function getLinearGraphDescription(
state: LinearGraphState,
i18n: I18nContextType,
) {
const strings = describeLinearGraph(state, i18n);

return strings.srLinearInteractiveElement;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import {mockPerseusI18nContext} from "../../../components/i18n-context";

import {describePointGraph} from "./point";
import {getPointGraphDescription} from "./point";

import type {PointGraphState} from "../types";

describe("describePointGraph", () => {
describe("getPointGraphDescription", () => {
const baseState: PointGraphState = {
type: "point",
coords: [],
Expand All @@ -22,14 +22,14 @@ describe("describePointGraph", () => {

it(`returns "No interactive elements" for a graph with no points`, () => {
const state: PointGraphState = {...baseState, coords: []};
expect(describePointGraph(state, mockPerseusI18nContext)).toBe(
expect(getPointGraphDescription(state, mockPerseusI18nContext)).toBe(
"No interactive elements",
);
});

it("describes one point", () => {
const state: PointGraphState = {...baseState, coords: [[3, 5]]};
expect(describePointGraph(state, mockPerseusI18nContext)).toBe(
expect(getPointGraphDescription(state, mockPerseusI18nContext)).toBe(
"Interactive elements: Point 1 at 3 comma 5.",
);
});
Expand All @@ -42,7 +42,7 @@ describe("describePointGraph", () => {
[2, 4],
],
};
expect(describePointGraph(state, mockPerseusI18nContext)).toBe(
expect(getPointGraphDescription(state, mockPerseusI18nContext)).toBe(
"Interactive elements: Point 1 at 3 comma 5. Point 2 at 2 comma 4.",
);
});
Expand All @@ -52,7 +52,7 @@ describe("describePointGraph", () => {
...baseState,
coords: [[-1.1234, 3.5678]],
};
expect(describePointGraph(state, mockPerseusI18nContext)).toBe(
expect(getPointGraphDescription(state, mockPerseusI18nContext)).toBe(
"Interactive elements: Point 1 at -1.123 comma 3.568.",
);
});
Expand Down
14 changes: 4 additions & 10 deletions packages/perseus/src/widgets/interactive-graphs/graphs/point.tsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import * as React from "react";

import {usePerseusI18n} from "../../../components/i18n-context";
import {actions} from "../reducer/interactive-graph-action";
import useGraphConfig from "../reducer/use-graph-config";

import {MovablePoint} from "./components/movable-point";
import {srFormatNumber} from "./screenreader-text";
import {useTransformVectorsToPixels, pixelsToVectors} from "./use-transform";

import type {I18nContextType} from "../../../components/i18n-context";
import type {PerseusStrings} from "../../../strings";
import type {GraphConfig} from "../reducer/use-graph-config";
import type {
Expand All @@ -20,10 +20,11 @@ import type {
export function renderPointGraph(
state: PointGraphState,
dispatch: Dispatch,
i18n: I18nContextType,
): InteractiveGraphElementSuite {
return {
graph: <PointGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: <PointGraphDescription state={state} />,
interactiveElementsDescription: getPointGraphDescription(state, i18n),
};
}

Expand Down Expand Up @@ -153,15 +154,8 @@ function UnlimitedPointGraph(statefulProps: StatefulProps) {
);
}

function PointGraphDescription({state}: {state: PointGraphState}) {
// PointGraphDescription needs to `usePerseusI18n`, so it has to be a
// component rather than a function that simply returns a string.
const i18n = usePerseusI18n();
return describePointGraph(state, i18n);
}

// Exported for testing
export function describePointGraph(
export function getPointGraphDescription(
state: PointGraphState,
i18n: {strings: PerseusStrings; locale: string},
): string {
Expand Down
10 changes: 3 additions & 7 deletions packages/perseus/src/widgets/interactive-graphs/graphs/ray.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ import type {vec} from "mafs";
export function renderRayGraph(
state: RayGraphState,
dispatch: Dispatch,
i18n: I18nContextType,
): InteractiveGraphElementSuite {
return {
graph: <RayGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: <RayGraphDescription state={state} />,
interactiveElementsDescription: getRayGraphDescription(state, i18n),
};
}

Expand Down Expand Up @@ -80,13 +81,8 @@ const RayGraph = (props: Props) => {
);
};

function RayGraphDescription({state}: {state: RayGraphState}) {
// The reason that RayGraphDescription is a component (rather than a
// function that returns a string) is because it needs to use a
// hook: `usePerseusI18n`.
const i18n = usePerseusI18n();
function getRayGraphDescription(state: RayGraphState, i18n: I18nContextType) {
const strings = describeRayGraph(state, i18n);

return strings.srRayInteractiveElement;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {mockPerseusI18nContext} from "../../../components/i18n-context";
import {MafsGraph} from "../mafs-graph";
import {getBaseMafsGraphPropsForTests} from "../utils";

import {describeSegmentGraph} from "./segment";
import {getSegmentGraphDescription} from "./segment";

import type {InteractiveGraphState} from "../types";
import type {UserEvent} from "@testing-library/user-event";
Expand Down Expand Up @@ -338,12 +338,12 @@ describe("Segment graph screen reader", () => {
);
});

describe("describeSegmentGraph", () => {
describe("getSegmentGraphDescription", () => {
test("describes a single segment", () => {
// Arrange

// Act
const interactiveElementsString = describeSegmentGraph(
const interactiveElementsString = getSegmentGraphDescription(
baseSingleSegmentState,
mockPerseusI18nContext,
);
Expand All @@ -358,7 +358,7 @@ describe("describeSegmentGraph", () => {
// Arrange

// Act
const interactiveElementsString = describeSegmentGraph(
const interactiveElementsString = getSegmentGraphDescription(
baseMultipleSegmentState,
mockPerseusI18nContext,
);
Expand All @@ -373,7 +373,7 @@ describe("describeSegmentGraph", () => {
// Arrange

// Act
const interactiveElementsString = describeSegmentGraph(
const interactiveElementsString = getSegmentGraphDescription(
{
...baseSingleSegmentState,
coords: [
Expand All @@ -396,7 +396,7 @@ describe("describeSegmentGraph", () => {
// Arrange

// Act
const interactiveElementsString = describeSegmentGraph(
const interactiveElementsString = getSegmentGraphDescription(
{
...baseMultipleSegmentState,
coords: [
Expand Down
15 changes: 3 additions & 12 deletions packages/perseus/src/widgets/interactive-graphs/graphs/segment.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,11 @@ import type {vec} from "mafs";
export function renderSegmentGraph(
state: SegmentGraphState,
dispatch: Dispatch,
i18n: I18nContextType,
): InteractiveGraphElementSuite {
return {
graph: <SegmentGraph graphState={state} dispatch={dispatch} />,
interactiveElementsDescription: (
<SegmentGraphDescription state={state} />
),
interactiveElementsDescription: getSegmentGraphDescription(state, i18n),
};
}

Expand Down Expand Up @@ -171,16 +170,8 @@ function getLengthOfSegment(segment: PairOfPoints) {
return kpoint.distanceToPoint(...segment);
}

function SegmentGraphDescription({state}: {state: SegmentGraphState}) {
// The reason that SegmentGraphDescription is a component (rather than a
// function that returns a string) is because it needs to use a
// hook: `usePerseusI18n`.
const i18n = usePerseusI18n();
return describeSegmentGraph(state, i18n);
}

// Exported for testing
export function describeSegmentGraph(
export function getSegmentGraphDescription(
state: SegmentGraphState,
i18n: I18nContextType,
): string {
Expand Down
12 changes: 6 additions & 6 deletions packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -710,19 +710,19 @@ const renderGraphElements = (props: {
case "angle":
return renderAngleGraph(state, dispatch);
case "segment":
return renderSegmentGraph(state, dispatch);
return renderSegmentGraph(state, dispatch, i18n);
case "linear-system":
return renderLinearSystemGraph(state, dispatch);
return renderLinearSystemGraph(state, dispatch, i18n);
case "linear":
return renderLinearGraph(state, dispatch);
return renderLinearGraph(state, dispatch, i18n);
case "ray":
return renderRayGraph(state, dispatch);
return renderRayGraph(state, dispatch, i18n);
case "polygon":
return renderPolygonGraph(state, dispatch, i18n, markings);
case "point":
return renderPointGraph(state, dispatch);
return renderPointGraph(state, dispatch, i18n);
case "circle":
return renderCircleGraph(state, dispatch);
return renderCircleGraph(state, dispatch, i18n);
case "quadratic":
return renderQuadraticGraph(state, dispatch, i18n);
case "sinusoid":
Expand Down

0 comments on commit 91cede4

Please sign in to comment.