From c04d275defbd73e2518c92859a2ad7a5f0e914ea Mon Sep 17 00:00:00 2001 From: Ben Christel Date: Mon, 23 Sep 2024 16:32:27 -0700 Subject: [PATCH] Omit extra properties from InteractiveGraph onChange arg (#1654) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, we were passing the entire Mafs graph state to onChange. Since the onChange callback data is used by the Interactive Graph widget editor to set the correct answer for the graph, a bunch of extra data like `"hasBeenInteractedWith": true` was getting saved in the `correct` field of the Widget data. This extra data is not used, so its presence in datastore is potentially confusing to future maintainers trying to understand the data. It might also cause bugs in the future - e.g. if some code merges it into another object in which those properties _are_ meaningful. This PR ensures that we only save relevant data in the `correct` field of interactive graph widgets. Issue: none ## Test plan: Edit, save and publish an interactive graph widget in an exercise. As a learner, you should be able to complete the exercise successfully. Author: benchristel Reviewers: benchristel, jeremywiebe, nishasy Required Reviewers: Approved By: jeremywiebe Checks: ❌ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: https://github.com/Khan/perseus/pull/1654 --- .changeset/eleven-elephants-trade.md | 5 + packages/perseus/src/types.ts | 4 +- .../perseus/src/widgets/interactive-graph.tsx | 1 - .../mafs-state-to-interactive-graph.test.ts | 288 ++++++++++++++++++ .../mafs-state-to-interactive-graph.ts | 60 ++++ .../stateful-mafs-graph.tsx | 24 +- 6 files changed, 358 insertions(+), 24 deletions(-) create mode 100644 .changeset/eleven-elephants-trade.md create mode 100644 packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.test.ts create mode 100644 packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts diff --git a/.changeset/eleven-elephants-trade.md b/.changeset/eleven-elephants-trade.md new file mode 100644 index 0000000000..9e79c16ece --- /dev/null +++ b/.changeset/eleven-elephants-trade.md @@ -0,0 +1,5 @@ +--- +"@khanacademy/perseus": patch +--- + +Omit unused data from interactive graph onChange callback diff --git a/packages/perseus/src/types.ts b/packages/perseus/src/types.ts index 2c848edd31..0ca8712620 100644 --- a/packages/perseus/src/types.ts +++ b/packages/perseus/src/types.ts @@ -3,12 +3,12 @@ import type {Item} from "./multi-items/item-types"; import type { Hint, PerseusAnswerArea, + PerseusGraphType, PerseusWidget, PerseusWidgetsMap, } from "./perseus-types"; import type {PerseusStrings} from "./strings"; import type {SizeClass} from "./util/sizing-utils"; -import type {InteractiveGraphState} from "./widgets/interactive-graphs/types"; import type {KeypadAPI} from "@khanacademy/math-input"; import type {AnalyticsEventHandlerFn} from "@khanacademy/perseus-core"; import type {LinterContextProps} from "@khanacademy/perseus-linter"; @@ -90,7 +90,7 @@ export type ChangeHandler = ( // perseus-all-package/widgets/grapher.jsx plot?: any; // Interactive Graph callback (see legacy: interactive-graph.tsx) - graph?: InteractiveGraphState; + graph?: PerseusGraphType; }, callback?: () => unknown | null | undefined, silent?: boolean, diff --git a/packages/perseus/src/widgets/interactive-graph.tsx b/packages/perseus/src/widgets/interactive-graph.tsx index e4f2626b42..b5e4226191 100644 --- a/packages/perseus/src/widgets/interactive-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graph.tsx @@ -1656,7 +1656,6 @@ class LegacyInteractiveGraph extends React.Component { $(this.angle).on("move", () => { this.onChange({ - // @ts-expect-error Type '{ coords: any; type: "angle"; showAngles?: boolean | undefined; allowReflexAngles?: boolean | undefined; angleOffsetDeg?: number | undefined; snapDegrees?: number | undefined; match?: "congruent" | undefined; }' is not assignable to type 'InteractiveGraphState | undefined'. graph: {...graph, coords: this.angle?.getClockwiseCoords()}, }); }); diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.test.ts b/packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.test.ts new file mode 100644 index 0000000000..3674ef8d8a --- /dev/null +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.test.ts @@ -0,0 +1,288 @@ +import {mafsStateToInteractiveGraph} from "./mafs-state-to-interactive-graph"; + +import type { + AngleGraphState, + CircleGraphState, + InteractiveGraphStateCommon, + LinearGraphState, + LinearSystemGraphState, + NoneGraphState, + PointGraphState, + PolygonGraphState, + QuadraticGraphState, + RayGraphState, + SegmentGraphState, + SinusoidGraphState, +} from "./types"; +import type {PerseusGraphType} from "../../perseus-types"; + +const commonGraphState: InteractiveGraphStateCommon = { + hasBeenInteractedWith: true, + range: [ + [-9, 9], + [-9, 9], + ], + snapStep: [9, 9], +}; + +describe("mafsStateToInteractiveGraph", () => { + it("converts the state of an angle graph", () => { + const state: AngleGraphState = { + ...commonGraphState, + type: "angle", + showAngles: true, + allowReflexAngles: true, + angleOffsetDeg: 7, + snapDegrees: 8, + coords: [ + [9, 10], + [11, 12], + [13, 14], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "angle", + coords: [ + [9, 10], + [11, 12], + [13, 14], + ], + }); + }); + + it("converts the state of a circle graph", () => { + const state: CircleGraphState = { + type: "circle", + center: [1, 2], + radiusPoint: [3, 2], + hasBeenInteractedWith: true, + range: [ + [-9, 9], + [-9, 9], + ], + snapStep: [9, 9], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "circle", + radius: 2, + center: [1, 2], + }); + }); + + it("converts the state of a segment graph", () => { + const state: SegmentGraphState = { + ...commonGraphState, + type: "segment", + coords: [ + [ + [1, 2], + [3, 4], + ], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "segment", + coords: [ + [ + [1, 2], + [3, 4], + ], + ], + }); + }); + + it("converts the state of a linear graph", () => { + const state: LinearGraphState = { + ...commonGraphState, + type: "linear", + coords: [ + [1, 2], + [3, 4], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "linear", + coords: [ + [1, 2], + [3, 4], + ], + }); + }); + + it("converts the state of a linear-system graph", () => { + const state: LinearSystemGraphState = { + ...commonGraphState, + type: "linear-system", + coords: [ + [ + [1, 2], + [3, 4], + ], + [ + [5, 6], + [7, 8], + ], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "linear-system", + coords: [ + [ + [1, 2], + [3, 4], + ], + [ + [5, 6], + [7, 8], + ], + ], + }); + }); + + it("converts the state of a ray graph", () => { + const state: RayGraphState = { + ...commonGraphState, + type: "ray", + coords: [ + [1, 2], + [3, 4], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "ray", + coords: [ + [1, 2], + [3, 4], + ], + }); + }); + + it("converts the state of a polygon graph", () => { + const state: PolygonGraphState = { + ...commonGraphState, + type: "polygon", + showAngles: true, + showSides: true, + snapTo: "sides", + coords: [ + [1, 2], + [3, 4], + [5, 6], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "polygon", + coords: [ + [1, 2], + [3, 4], + [5, 6], + ], + }); + }); + + it("converts the state of a point graph", () => { + const state: PointGraphState = { + ...commonGraphState, + type: "point", + numPoints: "unlimited", + focusedPointIndex: 99, + showRemovePointButton: true, + showKeyboardInteractionInvitation: true, + interactionMode: "mouse", + coords: [ + [1, 2], + [3, 4], + [5, 6], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "point", + coords: [ + [1, 2], + [3, 4], + [5, 6], + ], + }); + }); + + it("converts the state of a quadratic graph", () => { + const state: QuadraticGraphState = { + ...commonGraphState, + type: "quadratic", + coords: [ + [1, 2], + [3, 4], + [5, 6], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "quadratic", + coords: [ + [1, 2], + [3, 4], + [5, 6], + ], + }); + }); + + it("converts the state of a sinusoid graph", () => { + const state: SinusoidGraphState = { + ...commonGraphState, + type: "sinusoid", + coords: [ + [1, 2], + [3, 4], + ], + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "sinusoid", + coords: [ + [1, 2], + [3, 4], + ], + }); + }); + + it("converts the state of a none-type graph", () => { + const state: NoneGraphState = { + ...commonGraphState, + type: "none", + }; + + const result: PerseusGraphType = mafsStateToInteractiveGraph(state); + + expect(result).toEqual({ + type: "none", + }); + }); +}); diff --git a/packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts b/packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts new file mode 100644 index 0000000000..389965f9a8 --- /dev/null +++ b/packages/perseus/src/widgets/interactive-graphs/mafs-state-to-interactive-graph.ts @@ -0,0 +1,60 @@ +import {UnreachableCaseError} from "@khanacademy/wonder-stuff-core"; + +import {getRadius} from "./reducer/interactive-graph-state"; + +import type {InteractiveGraphState} from "./types"; +import type {PerseusGraphType} from "@khanacademy/perseus"; + +// Converts the state of a StatefulMafsGraph back to the format used to +// represent graph state in the widget JSON. +// +// Rather than be tightly bound to how data was structured in +// the legacy interactive graph, this lets us store state +// however we want and we just transform it before handing it off +// the the parent InteractiveGraph. +// +// The transformed data is used in the interactive graph widget editor, to +// set the `correct` field of the graph options. In the learner-facing UI, the +// data is also stored by the Renderer, and passed back down to the graph +// widget via the `graph` prop. +export function mafsStateToInteractiveGraph( + state: InteractiveGraphState, +): PerseusGraphType { + switch (state.type) { + case "angle": + case "quadratic": + return { + type: state.type, + coords: state.coords, + }; + case "circle": + return { + type: "circle", + center: state.center, + radius: getRadius(state), + }; + case "linear": + case "ray": + case "sinusoid": + return { + type: state.type, + coords: state.coords, + }; + case "segment": + case "linear-system": + return { + type: state.type, + coords: state.coords, + }; + case "polygon": + case "point": + return { + type: state.type, + coords: state.coords, + }; + case "none": + return {type: "none"}; + default: + throw new UnreachableCaseError(state); + } +} diff --git a/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx b/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx index c53f804f33..938b2443bc 100644 --- a/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx +++ b/packages/perseus/src/widgets/interactive-graphs/stateful-mafs-graph.tsx @@ -3,6 +3,7 @@ import * as React from "react"; import {useEffect, useImperativeHandle, useRef} from "react"; import {MafsGraph} from "./mafs-graph"; +import {mafsStateToInteractiveGraph} from "./mafs-state-to-interactive-graph"; import {initializeGraphState} from "./reducer/initialize-graph-state"; import { changeRange, @@ -10,7 +11,7 @@ import { reinitialize, } from "./reducer/interactive-graph-action"; import {interactiveGraphReducer} from "./reducer/interactive-graph-reducer"; -import {getGradableGraph, getRadius} from "./reducer/interactive-graph-state"; +import {getGradableGraph} from "./reducer/interactive-graph-state"; import type {InteractiveGraphProps, InteractiveGraphState} from "./types"; import type {PerseusGraphType} from "../../perseus-types"; @@ -41,25 +42,6 @@ export type StatefulMafsGraphProps = { static: InteractiveGraphProps["static"]; }; -// Rather than be tightly bound to how data was structured in -// the legacy interactive graph, this lets us store state -// however we want and we just transform it before handing it off -// the the parent InteractiveGraph -function mafsStateToInteractiveGraph(state: {graph: InteractiveGraphState}) { - if (state.graph.type === "circle") { - return { - ...state, - graph: { - ...state.graph, - radius: getRadius(state.graph), - }, - }; - } - return { - ...state, - }; -} - export const StatefulMafsGraph = React.forwardRef< Partial, StatefulMafsGraphProps @@ -80,7 +62,7 @@ export const StatefulMafsGraph = React.forwardRef< useEffect(() => { if (prevState.current !== state) { - onChange(mafsStateToInteractiveGraph({graph: state})); + onChange({graph: mafsStateToInteractiveGraph(state)}); } prevState.current = state; }, [onChange, state]);