Skip to content

Commit

Permalink
Omit extra properties from InteractiveGraph onChange arg (#1654)
Browse files Browse the repository at this point in the history
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: #1654
  • Loading branch information
benchristel authored Sep 23, 2024
1 parent ebf97db commit c04d275
Show file tree
Hide file tree
Showing 6 changed files with 358 additions and 24 deletions.
5 changes: 5 additions & 0 deletions .changeset/eleven-elephants-trade.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Omit unused data from interactive graph onChange callback
4 changes: 2 additions & 2 deletions packages/perseus/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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,
Expand Down
1 change: 0 additions & 1 deletion packages/perseus/src/widgets/interactive-graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1656,7 +1656,6 @@ class LegacyInteractiveGraph extends React.Component<Props, State> {

$(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()},
});
});
Expand Down
Original file line number Diff line number Diff line change
@@ -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",
});
});
});
Original file line number Diff line number Diff line change
@@ -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);
}
}
Loading

0 comments on commit c04d275

Please sign in to comment.