Skip to content

Commit

Permalink
Reset interactive graph state when the graph type or number of segmen…
Browse files Browse the repository at this point in the history
…ts changes (#1345)

## Summary:
There was a bug in the exercise editor where the preview did not reflect
changes to the graph type or number of segments. This happened because
changes to the `graph.type` and `graph.numSegments` props were ignored.
We now reinitialize the graph state when those props change.

Issue: LEMS-2041

Test plan:

Deploy a dev build of Perseus into the a webapp ZND. Visit:

https://www.khanacademy.org/devadmin/content/exercises/linear-graph-exercise/x3239cabc044aa28b/x325a92c13bc37711

Change the graph type of one of the interactive graphs to "segment".
The preview on the right-hand side of the editor should update.

Change the number of segments on the graph. The preview should update.

Author: benchristel

Reviewers: benchristel, nishasy, Myranae, #perseus, mark-fitzgerald, SonicScrewdriver, handeyeco

Required Reviewers:

Approved By: nishasy

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

Pull Request URL: #1345
  • Loading branch information
benchristel committed Jun 17, 2024
1 parent 9a6517c commit 92990f1
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 13 deletions.
5 changes: 5 additions & 0 deletions .changeset/chilled-hats-tan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus": patch
---

Fix a bug in the exercise editor where the preview did not update after a change to the graph type or number of line segments.
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,47 @@ describe("StatefulMafsGraph", () => {

expect(mockChangeHandler).toHaveBeenCalled();
});

it("re-renders when the graph type changes", () => {
// Arrange: render a segment graph
const segmentGraphProps: StatefulMafsGraphProps = {
...getBaseStatefulMafsGraphProps(),
graph: {type: "segment"},
};
const {rerender} = render(<StatefulMafsGraph {...segmentGraphProps} />);

// Act: rerender with a quadratic graph
const quadraticGraphProps: StatefulMafsGraphProps = {
...getBaseStatefulMafsGraphProps(),
graph: {type: "quadratic"},
};
rerender(<StatefulMafsGraph {...quadraticGraphProps} />);

// Assert: there should be 3 movable points (which define the quadratic
// function). If there are 2 points, it means we are still rendering
// the segment graph.
expect(screen.getAllByTestId("movable-point").length).toBe(3);
});

it("re-renders when the number of line segments on a segment graph changes", () => {
// Arrange: render a segment graph with one segment
const oneSegmentProps: StatefulMafsGraphProps = {
...getBaseStatefulMafsGraphProps(),
graph: {type: "segment", numSegments: 1},
};
const {rerender} = render(<StatefulMafsGraph {...oneSegmentProps} />);

// Act: rerender with two segments
const twoSegmentProps: StatefulMafsGraphProps = {
...getBaseStatefulMafsGraphProps(),
graph: {type: "segment", numSegments: 2},
};
rerender(<StatefulMafsGraph {...twoSegmentProps} />);

// Assert: there should be 4 movable points. If there are 2 points, it
// means we are still rendering a single segment.
expect(screen.getAllByTestId("movable-point").length).toBe(4);
});
});

function graphToPixel(
Expand Down
23 changes: 19 additions & 4 deletions packages/perseus/src/widgets/interactive-graphs/mafs-graph.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {View} from "@khanacademy/wonder-blocks-core";
import {useLatestRef, View} from "@khanacademy/wonder-blocks-core";
import {UnreachableCaseError} from "@khanacademy/wonder-stuff-core";
import {Mafs} from "mafs";
import * as React from "react";
Expand All @@ -25,13 +25,15 @@ import {initializeGraphState} from "./reducer/initialize-graph-state";
import {
changeRange,
changeSnapStep,
reinitialize,
type InteractiveGraphAction,
} from "./reducer/interactive-graph-action";
import {interactiveGraphReducer} from "./reducer/interactive-graph-reducer";
import {getGradableGraph, getRadius} from "./reducer/interactive-graph-state";
import {GraphConfigContext} from "./reducer/use-graph-config";

import type {InteractiveGraphState, InteractiveGraphProps} from "./types";
import type {PerseusGraphType} from "../../perseus-types";
import type {Widget} from "../../renderer";
import type {vec} from "mafs";

Expand All @@ -41,7 +43,7 @@ import "./mafs-styles.css";
export type StatefulMafsGraphProps = {
box: [number, number];
backgroundImage?: InteractiveGraphProps["backgroundImage"];
graph: InteractiveGraphProps["graph"];
graph: PerseusGraphType;
lockedFigures?: InteractiveGraphProps["lockedFigures"];
range: InteractiveGraphProps["range"];
snapStep: InteractiveGraphProps["snapStep"];
Expand Down Expand Up @@ -111,7 +113,7 @@ export const StatefulMafsGraph = React.forwardRef<
Partial<Widget>,
StatefulMafsGraphProps
>((props, ref) => {
const {onChange} = props;
const {onChange, graph} = props;

const [state, dispatch] = React.useReducer(
interactiveGraphReducer,
Expand All @@ -120,7 +122,7 @@ export const StatefulMafsGraph = React.forwardRef<
);

useImperativeHandle(ref, () => ({
getUserInput: () => getGradableGraph(state, props.graph),
getUserInput: () => getGradableGraph(state, graph),
}));

const prevState = useRef<InteractiveGraphState>(state);
Expand Down Expand Up @@ -149,6 +151,19 @@ export const StatefulMafsGraph = React.forwardRef<
);
}, [dispatch, xMinRange, xMaxRange, yMinRange, yMaxRange]);

const numSegments = graph.type === "segment" ? graph.numSegments : null;
const originalPropsRef = useRef(props);
const latestPropsRef = useLatestRef(props);
useEffect(() => {
// This conditional prevents the state from being "reinitialized" right
// after the first render. This is an optimization, but also prevents
// a bug where the graph would be marked "incorrect" during grading
// even if the user never interacted with it.
if (latestPropsRef.current !== originalPropsRef.current) {
dispatch(reinitialize(latestPropsRef.current));
}
}, [graph.type, numSegments, latestPropsRef]);

return <MafsGraph {...props} state={state} dispatch={dispatch} />;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,16 @@ import type {InteractiveGraphState, PairOfPoints} from "../types";
import type {Coord} from "@khanacademy/perseus";
import type {Interval} from "mafs";

export function initializeGraphState(params: {
export type InitializeGraphStateParams = {
range: [x: Interval, y: Interval];
step: [x: number, y: number];
snapStep: [x: number, y: number];
graph: PerseusGraphType;
}): InteractiveGraphState {
};

export function initializeGraphState(
params: InitializeGraphStateParams,
): InteractiveGraphState {
const {graph, step, snapStep, range} = params;
const shared = {
hasBeenInteractedWith: false,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import type {InitializeGraphStateParams} from "./initialize-graph-state";
import type {Interval, vec} from "mafs";

export type InteractiveGraphAction =
| Reinitialize
| MoveControlPoint
| MoveLine
| MoveAll
Expand All @@ -10,6 +12,18 @@ export type InteractiveGraphAction =
| ChangeSnapStep
| ChangeRange;

export const REINITIALIZE = "reinitialize";
export interface Reinitialize {
type: typeof REINITIALIZE;
params: InitializeGraphStateParams;
}
export function reinitialize(params: InitializeGraphStateParams): Reinitialize {
return {
type: REINITIALIZE,
params,
};
}

export const MOVE_CONTROL_POINT = "move-control-point";
export interface MoveControlPoint {
type: typeof MOVE_CONTROL_POINT;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,26 @@ import {polar} from "../../../util/graphie";
import {getQuadraticCoefficients} from "../graphs/quadratic";
import {snap} from "../utils";

import {initializeGraphState} from "./initialize-graph-state";
import {
CHANGE_RANGE,
CHANGE_SNAP_STEP,
type ChangeRange,
type ChangeSnapStep,
type InteractiveGraphAction,
MOVE_ALL,
MOVE_CENTER,
MOVE_CONTROL_POINT,
MOVE_LINE,
MOVE_POINT,
CHANGE_SNAP_STEP,
CHANGE_RANGE,
MOVE_CENTER,
MOVE_RADIUS_POINT,
type MoveAll,
type MoveCenter,
type MoveControlPoint,
type MoveLine,
type MoveCenter,
type MoveRadiusPoint,
type MovePoint,
type ChangeSnapStep,
type ChangeRange,
type MoveRadiusPoint,
REINITIALIZE,
} from "./interactive-graph-action";

import type {QuadraticCoords} from "../graphs/quadratic";
Expand All @@ -48,6 +50,8 @@ export function interactiveGraphReducer(
action: InteractiveGraphAction,
): InteractiveGraphState {
switch (action.type) {
case REINITIALIZE:
return initializeGraphState(action.params);
case MOVE_CONTROL_POINT:
return doMoveControlPoint(state, action);
case MOVE_LINE:
Expand Down

0 comments on commit 92990f1

Please sign in to comment.