-
Notifications
You must be signed in to change notification settings - Fork 364
docs(changeset): Removing Interactive Graph Builder and refactoring tests/storybook to use more standardized generator patterns. #3415
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
0f2d14a
6732df4
e1229cb
9bd7723
7257900
9fb6ad4
916c661
a171978
8069d08
4eb0338
b62d922
b82456b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@khanacademy/perseus": patch | ||
| "@khanacademy/perseus-core": patch | ||
| "@khanacademy/perseus-editor": patch | ||
| --- | ||
|
|
||
| Removing Interactive Graph Builder and refactoring tests/storybook to use more standardized generator patterns. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| import { | ||
| generateIGAbsoluteValueGraph, | ||
| generateIGAngleGraph, | ||
| generateIGCircleGraph, | ||
| generateIGExponentialGraph, | ||
| generateIGLinearGraph, | ||
| generateIGLinearSystemGraph, | ||
| generateIGLockedEllipse, | ||
|
|
@@ -19,6 +21,7 @@ import { | |
| generateIGSinusoidGraph, | ||
| generateIGTangentGraph, | ||
| generateInteractiveGraphOptions, | ||
| generateInteractiveGraphQuestion, | ||
| generateInteractiveGraphWidget, | ||
| } from "./interactive-graph-widget-generator"; | ||
|
|
||
|
|
@@ -1284,3 +1287,149 @@ describe("generateIGLockedLabel", () => { | |
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("generateIGExponentialGraph", () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decided to round out the tests for the new graphs while I was in here. |
||
| it("builds a default exponential graph", () => { | ||
| // Arrange, Act | ||
| const exponentialGraph = generateIGExponentialGraph(); | ||
|
|
||
| // Assert | ||
| expect(exponentialGraph).toEqual({ | ||
| type: "exponential", | ||
| }); | ||
| }); | ||
|
|
||
| it("builds an exponential graph with all props", () => { | ||
| // Arrange, Act | ||
| const exponentialGraph = generateIGExponentialGraph({ | ||
| coords: [ | ||
| [0, 1], | ||
| [1, 3], | ||
| ], | ||
| asymptote: 0, | ||
| startCoords: { | ||
| coords: [ | ||
| [0, 1], | ||
| [1, 3], | ||
| ], | ||
| asymptote: 0, | ||
| }, | ||
| }); | ||
|
|
||
| // Assert | ||
| expect(exponentialGraph).toEqual({ | ||
| type: "exponential", | ||
| coords: [ | ||
| [0, 1], | ||
| [1, 3], | ||
| ], | ||
| asymptote: 0, | ||
| startCoords: { | ||
| coords: [ | ||
| [0, 1], | ||
| [1, 3], | ||
| ], | ||
| asymptote: 0, | ||
| }, | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("generateIGAbsoluteValueGraph", () => { | ||
| it("builds a default absolute-value graph", () => { | ||
| // Arrange, Act | ||
| const absoluteValueGraph = generateIGAbsoluteValueGraph(); | ||
|
|
||
| // Assert | ||
| expect(absoluteValueGraph).toEqual({ | ||
| type: "absolute-value", | ||
| }); | ||
| }); | ||
|
|
||
| it("builds an absolute-value graph with all props", () => { | ||
| // Arrange, Act | ||
| const absoluteValueGraph = generateIGAbsoluteValueGraph({ | ||
| coords: [ | ||
| [0, 0], | ||
| [1, 1], | ||
| ], | ||
| startCoords: [ | ||
| [0, 0], | ||
| [1, 1], | ||
| ], | ||
| }); | ||
|
|
||
| // Assert | ||
| expect(absoluteValueGraph).toEqual({ | ||
| type: "absolute-value", | ||
| coords: [ | ||
| [0, 0], | ||
| [1, 1], | ||
| ], | ||
| startCoords: [ | ||
| [0, 0], | ||
| [1, 1], | ||
| ], | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("generateInteractiveGraphQuestion", () => { | ||
| it("builds a default interactive graph question", () => { | ||
| // Arrange, Act | ||
| const question = generateInteractiveGraphQuestion(); | ||
|
|
||
| // Assert | ||
| expect(question.content).toBe("[[☃ interactive-graph 1]]"); | ||
| expect(question.widgets["interactive-graph 1"]).toBeDefined(); | ||
| expect(question.widgets["interactive-graph 1"].type).toBe( | ||
| "interactive-graph", | ||
| ); | ||
| }); | ||
|
|
||
| it("infers graph type from correct answer type", () => { | ||
| // Arrange, Act | ||
| const question = generateInteractiveGraphQuestion({ | ||
| correct: generateIGPointGraph({numPoints: 3}), | ||
| }); | ||
|
|
||
| // Assert | ||
| const options = question.widgets["interactive-graph 1"].options; | ||
| expect(options.correct.type).toBe("point"); | ||
| expect(options.graph.type).toBe("point"); | ||
| }); | ||
|
|
||
| it("does not override graph when explicitly provided", () => { | ||
| // Arrange, Act | ||
| const question = generateInteractiveGraphQuestion({ | ||
| correct: generateIGPointGraph({numPoints: 3}), | ||
| graph: generateIGPointGraph({numPoints: 5}), | ||
| }); | ||
|
|
||
| // Assert | ||
| const options = question.widgets["interactive-graph 1"].options; | ||
| expect(options.graph).toEqual({type: "point", numPoints: 5}); | ||
| }); | ||
|
|
||
| it("sets static mode when isStatic is true", () => { | ||
| // Arrange, Act | ||
| const question = generateInteractiveGraphQuestion({ | ||
| isStatic: true, | ||
| }); | ||
|
|
||
| // Assert | ||
| expect(question.widgets["interactive-graph 1"].static).toBe(true); | ||
| }); | ||
|
|
||
| it("uses custom content when provided", () => { | ||
| // Arrange, Act | ||
| const question = generateInteractiveGraphQuestion({ | ||
| content: "Custom content [[☃ interactive-graph 1]]", | ||
| }); | ||
|
|
||
| // Assert | ||
| expect(question.content).toBe( | ||
| "Custom content [[☃ interactive-graph 1]]", | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,6 @@ | ||
| import interactiveGraphWidgetLogic from "../../widgets/interactive-graph"; | ||
| import {getDefaultFigureForType} from "../get-default-figure-for-type"; | ||
| import {generateTestPerseusRenderer} from "../test-utils"; | ||
|
|
||
| import type { | ||
| InteractiveGraphWidget, | ||
|
|
@@ -10,8 +11,11 @@ | |
| LockedPointType, | ||
| LockedPolygonType, | ||
| LockedVectorType, | ||
| PerseusGraphType, | ||
| PerseusGraphTypeAbsoluteValue, | ||
| PerseusGraphTypeAngle, | ||
| PerseusGraphTypeCircle, | ||
| PerseusGraphTypeExponential, | ||
| PerseusGraphTypeLinear, | ||
| PerseusGraphTypeLinearSystem, | ||
| PerseusGraphTypeNone, | ||
|
|
@@ -23,6 +27,7 @@ | |
| PerseusGraphTypeSinusoid, | ||
| PerseusGraphTypeTangent, | ||
| PerseusInteractiveGraphWidgetOptions, | ||
| PerseusRenderer, | ||
| } from "../../data-schema"; | ||
|
|
||
| export function generateInteractiveGraphWidget( | ||
|
|
@@ -51,7 +56,7 @@ | |
| } | ||
|
|
||
| export function generateIGAngleGraph( | ||
| options?: Partial<PerseusGraphTypeAngle>, | ||
| options?: Partial<Omit<PerseusGraphTypeAngle, "type">>, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just some additional type safety that brings these generators inline with the implementation of the rest of the IG generators. |
||
| ): PerseusGraphTypeAngle { | ||
| return { | ||
| type: "angle", | ||
|
|
@@ -60,7 +65,7 @@ | |
| } | ||
|
|
||
| export function generateIGCircleGraph( | ||
| options?: Partial<PerseusGraphTypeCircle>, | ||
| options?: Partial<Omit<PerseusGraphTypeCircle, "type">>, | ||
| ): PerseusGraphTypeCircle { | ||
| return { | ||
| type: "circle", | ||
|
|
@@ -69,7 +74,7 @@ | |
| } | ||
|
|
||
| export function generateIGLinearGraph( | ||
| options?: Partial<PerseusGraphTypeLinear>, | ||
| options?: Partial<Omit<PerseusGraphTypeLinear, "type">>, | ||
| ): PerseusGraphTypeLinear { | ||
| return { | ||
| type: "linear", | ||
|
|
@@ -78,7 +83,7 @@ | |
| } | ||
|
|
||
| export function generateIGLinearSystemGraph( | ||
| options?: Partial<PerseusGraphTypeLinearSystem>, | ||
| options?: Partial<Omit<PerseusGraphTypeLinearSystem, "type">>, | ||
| ): PerseusGraphTypeLinearSystem { | ||
| return { | ||
| type: "linear-system", | ||
|
|
@@ -217,3 +222,63 @@ | |
| ...options, | ||
| }; | ||
| } | ||
|
|
||
| export function generateIGExponentialGraph( | ||
| options?: Partial<Omit<PerseusGraphTypeExponential, "type">>, | ||
| ): PerseusGraphTypeExponential { | ||
| return { | ||
| type: "exponential", | ||
| ...options, | ||
| }; | ||
| } | ||
|
|
||
| export function generateIGAbsoluteValueGraph( | ||
| options?: Partial<Omit<PerseusGraphTypeAbsoluteValue, "type">>, | ||
| ): PerseusGraphTypeAbsoluteValue { | ||
| return { | ||
| type: "absolute-value", | ||
| ...options, | ||
| }; | ||
| } | ||
|
|
||
| export function generateInteractiveGraphQuestion( | ||
| options?: Partial<PerseusInteractiveGraphWidgetOptions> & { | ||
| content?: string; | ||
| isStatic?: boolean; | ||
| }, | ||
| ): PerseusRenderer { | ||
| const {content, isStatic, ...widgetOptions} = options ?? {}; | ||
|
|
||
| // The `graph` and `correct` fields share all fields except for | ||
| // the answers (coords, center, etc.) When only `correct` is provided, | ||
| // derive `graph` from it by stripping the answer-specific fields. | ||
| // This allows us to keep our test data more succinct. | ||
| if (widgetOptions.correct && !widgetOptions.graph) { | ||
| const { | ||
| coords: _, | ||
| coord: __, | ||
| center: ___, | ||
| radius: ____, | ||
| asymptote: _____, | ||
| match: ______, | ||
| ...graphConfig | ||
| } = widgetOptions.correct as Record<string, unknown>; | ||
|
Check warning on line 265 in packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.ts
|
||
|
Comment on lines
+243
to
+265
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The developer architecture notes in Extended reasoning...What the bug is and how it manifestsThis PR deletes
The specific code path that triggers itThese notes are static Markdown documentation files intended to guide developers implementing new graph types. They list the files and functions a developer needs to touch, with Why existing code doesn't prevent itThe deletion of the builder was a source-code change; there is no automated tooling that cross-checks prose documentation for file-path accuracy. The notes were written to describe the old architecture and were not updated in this refactor. What the impact would beA developer tasked with adding a new interactive graph type (e.g., logarithm or tangent) would consult these notes, encounter the dead reference, and either waste time searching for the file or be left confused about where the builder logic now lives. The correct file is How to fix itIn both Step-by-step proof
|
||
| widgetOptions.graph = graphConfig as PerseusGraphType; | ||
| } | ||
|
|
||
| const optionsWithDefaults = { | ||
| gridStep: [1, 1] as [number, number], | ||
| snapStep: [0.5, 0.5] as [number, number], | ||
| ...widgetOptions, | ||
| }; | ||
|
|
||
| return generateTestPerseusRenderer({ | ||
| content: content ?? "[[☃ interactive-graph 1]]", | ||
| widgets: { | ||
| "interactive-graph 1": generateInteractiveGraphWidget({ | ||
| static: isStatic ?? false, | ||
| options: generateInteractiveGraphOptions(optionsWithDefaults), | ||
| }), | ||
claude[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }, | ||
| }); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It made sense to me to add a global override for our test data as it seems reasonable for these files to be exempt, but I'm also happy to remove this and re-add the file-specific override for interactive-graph.testdata.ts.