Skip to content

Commit

Permalink
[Interactive Graph Editor] Remove the use of graphKey for remounting (#…
Browse files Browse the repository at this point in the history
…1385)

## Summary:
It turns out that the use of `graphKey` for remounting in
`interactive-graph-editor.tsx` is redundant, since this is also being
done by the `useEffect` in `mafs-graph.tsx`.

`graphKey` was also causing a warning to show up in the logs, possibly
from a remounting loop.

Here, `graphKey` is being removed altogether to stop the remounting issue.

[More info on Slack](https://khanacademy.slack.com/archives/C067UM1QAR4/p1719854190583499)

Issue: none

## Test plan:
Storybook
- http://localhost:6006/?path=/story/perseuseditor-editorpage--demo
- Add an interactive graph
- Open the browser console
- Move the graph in the "correct" preview (quickly! Don't hold for time before dragging)
- Confirm the warning doesn't appear
- Change properties of the graph (e.g. number of segments)
- Confirm that the preview changes correctly
- Repeat for every graph type

Author: nishasy

Reviewers: benchristel, mark-fitzgerald, Myranae, SonicScrewdriver

Required Reviewers:

Approved By: benchristel

Checks: ✅ gerald, ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️  Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Jest Coverage (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), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x), ✅ gerald

Pull Request URL: #1385
  • Loading branch information
nishasy committed Jul 1, 2024
1 parent f280004 commit 30f898c
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 36 deletions.
5 changes: 5 additions & 0 deletions .changeset/heavy-buckets-think.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@khanacademy/perseus-editor": patch
---

[Interactive Graph Editor] Remove the use of graphKey for remounting
Original file line number Diff line number Diff line change
Expand Up @@ -617,21 +617,4 @@ describe("InteractiveGraphEditor", () => {
// Assert
expect(ref.current?.getSaveWarnings()).toEqual([]);
});

test("buildGraphKey returns the correct key", async () => {
// Arrange
const graph: PerseusGraphType = {
type: "polygon",
numSides: 4,
snapTo: "grid",
showAngles: true,
showSides: true,
};

// Act
const key = InteractiveGraphEditor.buildGraphKey(graph);

// Assert
expect(key).toEqual("polygon:4:grid:true:true");
});
});
19 changes: 0 additions & 19 deletions packages/perseus-editor/src/widgets/interactive-graph-editor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import type {
PerseusInteractiveGraphWidgetOptions,
APIOptionsWithDefaults,
LockedFigure,
PerseusGraphType,
} from "@khanacademy/perseus";
import type {PropsFor} from "@khanacademy/wonder-blocks-core";

Expand Down Expand Up @@ -199,20 +198,6 @@ class InteractiveGraphEditor extends React.Component<Props> {
DeprecationMixin.UNSAFE_componentWillMount.call(this);
}

static buildGraphKey(correct: PerseusGraphType) {
const testGraphKey: any[] = [];
for (const key in correct) {
if (correct[key]) {
typeof correct[key] === "number" ||
typeof correct[key] === "string" ||
typeof correct[key] === "boolean"
? testGraphKey.push(correct[key])
: testGraphKey.push(0);
}
}
return testGraphKey.join(":");
}

render() {
let graph;
let equationString;
Expand Down Expand Up @@ -262,9 +247,6 @@ class InteractiveGraphEditor extends React.Component<Props> {
this.props.onChange({correct: correct});
},
} as const;
// This is used to force a remount of the graph component
// when there's a significant change
const graphKey = InteractiveGraphEditor.buildGraphKey(correct);

graph = (
// There are a bunch of props that renderer.jsx passes to widgets via
Expand All @@ -273,7 +255,6 @@ class InteractiveGraphEditor extends React.Component<Props> {
// @ts-expect-error - TS2769 - No overload matches this call.
<InteractiveGraph
{...graphProps}
key={graphKey}
containerSizeClass={sizeClass}
apiOptions={{
...this.props.apiOptions,
Expand Down

0 comments on commit 30f898c

Please sign in to comment.