Skip to content

Commit ce19ec9

Browse files
SonicScrewdriverSonicScrewdriver
authored
Update the Interactive Graph Editor to use APIOptions to avoid a bug when saving (#3034)
## Summary: When setting up the new `editingDisabled` feature, I didn't realize that the changeable `onChange` event was taking ALL of the sub-component props and passing them up. Unfortunately, this results in the `editingDisabled` property being serialized when it shouldn't be. In order to resolve this, I've removed the named property in favour of using the value from `apiOptions` directly — as we already have logic to strip out the `apiOptions` using a prop deny list. This makes for a more consistent experience, and is better set up for when we move `apiOptions` into a context. ## Test plan: - Current tests pass - Interactive graph can be updated and saved correctly in the main Khan Academy environment. (Verified) Author: SonicScrewdriver Reviewers: nishasy, Myranae Required Reviewers: Approved By: nishasy Checks: ⏭️ 1 check has been skipped, ✅ 10 checks were successful Pull Request URL: #3034
1 parent c95b497 commit ce19ec9

File tree

7 files changed

+170
-48
lines changed

7 files changed

+170
-48
lines changed

‎.changeset/tame-coins-accept.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@khanacademy/perseus-editor": patch
3+
---
4+
5+
Fixing Interactive Graph Options due to Regression

‎packages/perseus-editor/src/components/__stories__/locked-figures-section.stories.tsx‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
import {ApiOptions} from "@khanacademy/perseus";
12
import {View} from "@khanacademy/wonder-blocks-core";
23
import {color, spacing} from "@khanacademy/wonder-blocks-tokens";
34
import {StyleSheet} from "aphrodite";
@@ -34,7 +35,7 @@ export const Controlled: StoryComponentType = {
3435
<LockedFiguresSection
3536
figures={figures}
3637
onChange={handlePropsUpdate}
37-
editingDisabled={false}
38+
apiOptions={ApiOptions.defaults}
3839
/>
3940
);
4041
},
@@ -56,7 +57,7 @@ export const WithProdWidth: StoryComponentType = {
5657
<LockedFiguresSection
5758
figures={figures}
5859
onChange={handlePropsUpdate}
59-
editingDisabled={false}
60+
apiOptions={ApiOptions.defaults}
6061
/>
6162
</View>
6263
);

‎packages/perseus-editor/src/widgets/interactive-graph-editor/components/interactive-graph-settings.test.tsx‎

Lines changed: 140 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import {Dependencies, Util} from "@khanacademy/perseus";
1+
import {ApiOptions, Dependencies, Util} from "@khanacademy/perseus";
22
import {render, screen, waitFor, fireEvent} from "@testing-library/react";
33
import {userEvent as userEventLib} from "@testing-library/user-event";
44
import * as React from "react";
@@ -36,15 +36,25 @@ describe("InteractiveGraphSettings", () => {
3636
test("common graph settings are shown, by default", async () => {
3737
// Arrange
3838
// Act
39-
render(<InteractiveGraphSettings onChange={() => {}} />);
39+
render(
40+
<InteractiveGraphSettings
41+
onChange={() => {}}
42+
apiOptions={ApiOptions.defaults}
43+
/>,
44+
);
4045

4146
// Assert
4247
expect(screen.getByText("x Label")).toBeVisible();
4348
});
4449

4550
test("hides common graph settings when heading clicked", async () => {
4651
// Arrange
47-
render(<InteractiveGraphSettings onChange={() => {}} />);
52+
render(
53+
<InteractiveGraphSettings
54+
onChange={() => {}}
55+
apiOptions={ApiOptions.defaults}
56+
/>,
57+
);
4858

4959
// Act
5060
await userEvent.click(screen.getByText("Common Graph Settings"));
@@ -55,7 +65,12 @@ describe("InteractiveGraphSettings", () => {
5565

5666
test("shows common graph settings when heading clicked a second time", async () => {
5767
// Arrange
58-
render(<InteractiveGraphSettings onChange={() => {}} />);
68+
render(
69+
<InteractiveGraphSettings
70+
onChange={() => {}}
71+
apiOptions={ApiOptions.defaults}
72+
/>,
73+
);
5974
await userEvent.click(screen.getByText("Common Graph Settings"));
6075

6176
// Act
@@ -68,7 +83,12 @@ describe("InteractiveGraphSettings", () => {
6883
test("displays graph, snap, image, and measure settings when common settings shown", async () => {
6984
// Arrange
7085
// Act
71-
render(<InteractiveGraphSettings onChange={() => {}} />);
86+
render(
87+
<InteractiveGraphSettings
88+
onChange={() => {}}
89+
apiOptions={ApiOptions.defaults}
90+
/>,
91+
);
7292

7393
// Assert
7494
expect(screen.getByText("x Label")).toBeInTheDocument();
@@ -82,7 +102,12 @@ describe("InteractiveGraphSettings", () => {
82102
test("calls onChange when markings are changed", async () => {
83103
// Arrange
84104
const onChange = jest.fn();
85-
render(<InteractiveGraphSettings onChange={onChange} />);
105+
render(
106+
<InteractiveGraphSettings
107+
onChange={onChange}
108+
apiOptions={ApiOptions.defaults}
109+
/>,
110+
);
86111

87112
// Act
88113
const button = screen.getByRole("button", {name: "Grid"});
@@ -105,7 +130,12 @@ describe("InteractiveGraphSettings", () => {
105130
};
106131
jest.spyOn(Util, "getImageSize").mockImplementation(mockGetImageSize);
107132

108-
render(<InteractiveGraphSettings onChange={onChange} />);
133+
render(
134+
<InteractiveGraphSettings
135+
onChange={onChange}
136+
apiOptions={ApiOptions.defaults}
137+
/>,
138+
);
109139

110140
// Act
111141
const input = screen.getByRole("textbox", {
@@ -138,7 +168,12 @@ describe("InteractiveGraphSettings", () => {
138168
};
139169
jest.spyOn(Util, "getImageSize").mockImplementation(mockGetImageSize);
140170

141-
render(<InteractiveGraphSettings onChange={onChange} />);
171+
render(
172+
<InteractiveGraphSettings
173+
onChange={onChange}
174+
apiOptions={ApiOptions.defaults}
175+
/>,
176+
);
142177

143178
// Act
144179
const input = screen.getByRole("textbox", {
@@ -166,7 +201,12 @@ describe("InteractiveGraphSettings", () => {
166201
const mockGetImageSize = (url, cb: (width, height) => void) => {};
167202
jest.spyOn(Util, "getImageSize").mockImplementation(mockGetImageSize);
168203

169-
render(<InteractiveGraphSettings onChange={onChange} />);
204+
render(
205+
<InteractiveGraphSettings
206+
onChange={onChange}
207+
apiOptions={ApiOptions.defaults}
208+
/>,
209+
);
170210

171211
// Act
172212
const input = screen.getByRole("textbox", {
@@ -194,7 +234,12 @@ describe("InteractiveGraphSettings", () => {
194234
userEvent = userEventForRealTimers();
195235
const onChange = jest.fn();
196236

197-
render(<InteractiveGraphSettings onChange={onChange} />);
237+
render(
238+
<InteractiveGraphSettings
239+
onChange={onChange}
240+
apiOptions={ApiOptions.defaults}
241+
/>,
242+
);
198243

199244
// Act
200245
const input = screen.getByRole("textbox", {
@@ -216,6 +261,7 @@ describe("InteractiveGraphSettings", () => {
216261
<InteractiveGraphSettings
217262
showProtractor
218263
onChange={() => undefined}
264+
apiOptions={ApiOptions.defaults}
219265
/>,
220266
);
221267

@@ -232,11 +278,17 @@ describe("InteractiveGraphSettings", () => {
232278
<InteractiveGraphSettings
233279
showProtractor
234280
onChange={() => undefined}
281+
apiOptions={ApiOptions.defaults}
235282
/>,
236283
);
237284

238285
// Act
239-
rerender(<InteractiveGraphSettings onChange={() => undefined} />);
286+
rerender(
287+
<InteractiveGraphSettings
288+
onChange={() => undefined}
289+
apiOptions={ApiOptions.defaults}
290+
/>,
291+
);
240292

241293
// Assert
242294
const banner = screen.queryByRole("alert");
@@ -250,6 +302,7 @@ describe("InteractiveGraphSettings", () => {
250302
<InteractiveGraphSettings
251303
showProtractor={true}
252304
onChange={onChange}
305+
apiOptions={ApiOptions.defaults}
253306
/>,
254307
);
255308

@@ -271,7 +324,12 @@ describe("InteractiveGraphSettings", () => {
271324
jest.useRealTimers();
272325
userEvent = userEventForRealTimers();
273326
const onChange = jest.fn();
274-
render(<InteractiveGraphSettings onChange={onChange} />);
327+
render(
328+
<InteractiveGraphSettings
329+
onChange={onChange}
330+
apiOptions={ApiOptions.defaults}
331+
/>,
332+
);
275333

276334
// Act
277335
// Note: The textbox's `name` attribute is "x Range 10" because it's
@@ -303,7 +361,12 @@ describe("InteractiveGraphSettings", () => {
303361
jest.useRealTimers();
304362
userEvent = userEventForRealTimers();
305363
const onChange = jest.fn();
306-
render(<InteractiveGraphSettings onChange={onChange} />);
364+
render(
365+
<InteractiveGraphSettings
366+
onChange={onChange}
367+
apiOptions={ApiOptions.defaults}
368+
/>,
369+
);
307370

308371
// Act
309372
const input = screen.getByRole("textbox", {name: "y Range 10"});
@@ -330,7 +393,12 @@ describe("InteractiveGraphSettings", () => {
330393
jest.useRealTimers();
331394
userEvent = userEventForRealTimers();
332395
const onChange = jest.fn();
333-
render(<InteractiveGraphSettings onChange={onChange} />);
396+
render(
397+
<InteractiveGraphSettings
398+
onChange={onChange}
399+
apiOptions={ApiOptions.defaults}
400+
/>,
401+
);
334402

335403
// Act
336404
const input = screen.getByRole("textbox", {name: "x Range 10"});
@@ -357,7 +425,12 @@ describe("InteractiveGraphSettings", () => {
357425
jest.useRealTimers();
358426
userEvent = userEventForRealTimers();
359427
const onChange = jest.fn();
360-
render(<InteractiveGraphSettings onChange={onChange} />);
428+
render(
429+
<InteractiveGraphSettings
430+
onChange={onChange}
431+
apiOptions={ApiOptions.defaults}
432+
/>,
433+
);
361434

362435
// Act
363436
const input = screen.getByRole("textbox", {name: "Tick Step 1"});
@@ -381,7 +454,12 @@ describe("InteractiveGraphSettings", () => {
381454
jest.useRealTimers();
382455
userEvent = userEventForRealTimers();
383456
const onChange = jest.fn();
384-
render(<InteractiveGraphSettings onChange={onChange} />);
457+
render(
458+
<InteractiveGraphSettings
459+
onChange={onChange}
460+
apiOptions={ApiOptions.defaults}
461+
/>,
462+
);
385463

386464
// Act
387465
const input = screen.getByRole("textbox", {name: "Tick Step 1"});
@@ -412,6 +490,7 @@ describe("InteractiveGraphSettings", () => {
412490
[-100, 100],
413491
]}
414492
onChange={onChange}
493+
apiOptions={ApiOptions.defaults}
415494
/>,
416495
);
417496

@@ -437,7 +516,12 @@ describe("InteractiveGraphSettings", () => {
437516
jest.useRealTimers();
438517
userEvent = userEventForRealTimers();
439518
const onChange = jest.fn();
440-
render(<InteractiveGraphSettings onChange={onChange} />);
519+
render(
520+
<InteractiveGraphSettings
521+
onChange={onChange}
522+
apiOptions={ApiOptions.defaults}
523+
/>,
524+
);
441525

442526
// Act
443527
const input = screen.getByRole("textbox", {name: "Snap Step 1"});
@@ -462,7 +546,12 @@ describe("InteractiveGraphSettings", () => {
462546
jest.useRealTimers();
463547
userEvent = userEventForRealTimers();
464548
const onChange = jest.fn();
465-
render(<InteractiveGraphSettings onChange={onChange} />);
549+
render(
550+
<InteractiveGraphSettings
551+
onChange={onChange}
552+
apiOptions={ApiOptions.defaults}
553+
/>,
554+
);
466555

467556
// Act
468557
const input = screen.getByRole("textbox", {name: "Snap Step 1"});
@@ -487,7 +576,12 @@ describe("InteractiveGraphSettings", () => {
487576
jest.useRealTimers();
488577
userEvent = userEventForRealTimers();
489578
const onChange = jest.fn();
490-
render(<InteractiveGraphSettings onChange={onChange} />);
579+
render(
580+
<InteractiveGraphSettings
581+
onChange={onChange}
582+
apiOptions={ApiOptions.defaults}
583+
/>,
584+
);
491585

492586
// Act
493587
const input = screen.getByRole("textbox", {name: "Grid Step 1"});
@@ -512,7 +606,12 @@ describe("InteractiveGraphSettings", () => {
512606
jest.useRealTimers();
513607
userEvent = userEventForRealTimers();
514608
const onChange = jest.fn();
515-
render(<InteractiveGraphSettings onChange={onChange} />);
609+
render(
610+
<InteractiveGraphSettings
611+
onChange={onChange}
612+
apiOptions={ApiOptions.defaults}
613+
/>,
614+
);
516615

517616
// Act
518617
const input = screen.getByRole("textbox", {name: "Grid Step 1"});
@@ -537,7 +636,12 @@ describe("InteractiveGraphSettings", () => {
537636
jest.useRealTimers();
538637
userEvent = userEventForRealTimers();
539638
const onChange = jest.fn();
540-
render(<InteractiveGraphSettings onChange={onChange} />);
639+
render(
640+
<InteractiveGraphSettings
641+
onChange={onChange}
642+
apiOptions={ApiOptions.defaults}
643+
/>,
644+
);
541645

542646
// Act
543647
const input = screen.getByRole("textbox", {name: "x Label"});
@@ -561,7 +665,12 @@ describe("InteractiveGraphSettings", () => {
561665
jest.useRealTimers();
562666
userEvent = userEventForRealTimers();
563667
const onChange = jest.fn();
564-
render(<InteractiveGraphSettings onChange={onChange} />);
668+
render(
669+
<InteractiveGraphSettings
670+
onChange={onChange}
671+
apiOptions={ApiOptions.defaults}
672+
/>,
673+
);
565674

566675
// Act
567676
const input = screen.getByRole("textbox", {name: "y Label"});
@@ -604,6 +713,7 @@ describe("InteractiveGraphSettings", () => {
604713
<InteractiveGraphSettings
605714
range={inputRange}
606715
onChange={onChange}
716+
apiOptions={ApiOptions.defaults}
607717
/>,
608718
);
609719

@@ -649,7 +759,12 @@ describe("InteractiveGraphSettings", () => {
649759
// Arrange
650760

651761
// Act
652-
render(<InteractiveGraphSettings onChange={() => {}} />);
762+
render(
763+
<InteractiveGraphSettings
764+
onChange={() => {}}
765+
apiOptions={ApiOptions.defaults}
766+
/>,
767+
);
653768

654769
// Assert
655770
expect(screen.getByRole("switch", {name: "x min"})).toBeChecked();
@@ -678,6 +793,7 @@ describe("InteractiveGraphSettings", () => {
678793
// Set the axis arrow being tested to false.
679794
[axis]: false,
680795
}}
796+
apiOptions={ApiOptions.defaults}
681797
/>,
682798
);
683799

@@ -705,6 +821,7 @@ describe("InteractiveGraphSettings", () => {
705821
<InteractiveGraphSettings
706822
onChange={onChange}
707823
showAxisArrows={defaultShowAxisArrows}
824+
apiOptions={ApiOptions.defaults}
708825
/>,
709826
);
710827

0 commit comments

Comments
 (0)