refactor: locked polygon points shape by mojadem · Pull Request #3705 · Khan/perseus

mojadem · 2026-06-01T20:55:07Z

Summary

This is the first PR in a stack to enable per-vertex angle labeling in locked polygons.

Add LockedPolygonPointType with shape {coord: Coord}
Change LockedPolygonType.points from type Coord[] to LockedPolygonPointType[]
Update rendering, editor, linter, defaults, fixtures, stories, and tests to use points.coord
Keep parser backwards-compatible by accepting legacy raw Coord[] polygon points and normalizing to the new shape
Add parser coverage for both new and legacy locked polygon point formats

Next steps

Extend LockedPolygonPointType with a showAngle field that determines whether the angle should be shown for that point (feat: add showAngle field to LockedPolygonPointType #3730)
Extend LockedPolygonPointType with a computedAngle field that is populated during parsing with the computed angle measurement

Test plan

pnpm jest packages/perseus-core/src/parse-perseus-json/regression-tests/parse-perseus-json-regression.test.ts -u --runInBand --watchman=false
pnpm jest packages/perseus-core/src/parse-perseus-json/perseus-parsers/interactive-graph-widget.test.ts packages/perseus-core/src/utils/get-default-figure-for-type.test.ts packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.test.ts packages/perseus-linter/src/rules/interactive-graph-widget-error.test.ts --runInBand --watchman=false

github-actions · 2026-06-01T20:57:34Z

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (815a548) and published it to npm. You
can install it using the tag PR3705.

Example:

pnpm add @khanacademy/perseus@PR3705

If you are working in Khan Academy's frontend, you can run the below command.

./dev/tools/bump_perseus_version.ts -t PR3705

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR3705

github-actions · 2026-06-01T20:58:15Z

Size Change: +184 B (+0.04%)

Total Size: 508 kB

📦 View Changed

Filename	Size	Change
`packages/perseus-core/dist/es/index.item-splitting.js`	12.1 kB	+51 B (+0.42%)
`packages/perseus-core/dist/es/index.js`	26.3 kB	+71 B (+0.27%)
`packages/perseus-editor/dist/es/index.js`	105 kB	+41 B (+0.04%)
`packages/perseus-linter/dist/es/index.js`	9.66 kB	+11 B (+0.11%)
`packages/perseus/dist/es/index.js`	200 kB	+10 B (0%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	20.6 kB
`packages/keypad-context/dist/es/index.js`	1 kB
`packages/kmath/dist/es/index.js`	6.32 kB
`packages/math-input/dist/es/index.js`	98.5 kB
`packages/math-input/dist/es/strings.js`	1.61 kB
`packages/perseus-score/dist/es/index.js`	10.2 kB
`packages/perseus-utils/dist/es/index.js`	403 B
`packages/perseus/dist/es/strings.js`	8.6 kB
`packages/pure-markdown/dist/es/index.js`	1.39 kB
`packages/simple-markdown/dist/es/index.js`	6.71 kB

_{compressed-size-action}

…ckedPolygonType.points` from `[]Coord` to `LockedPolygonPointType`

…tType to []LockedPolygonPointType

SonicScrewdriver

I think this looks good! Thanks for working on this. I'll also tag @jeremywiebe in case he has any further thoughts on the approach.

We'll need to be very careful deploying this, as we need to give the Content Platform a heads up about the schema change before we do. Otherwise we can break publishing. :) Happy to provide support with deploying if you need any!

SonicScrewdriver · 2026-06-03T17:00:32Z

 export type LockedPolygonType = {
    type: "polygon";
-    points: Coord[];
+    points: LockedPolygonPointType[];


It might be nice to add a comment here explaining the intention to add more data in the LockedPolygonPointType, but it's not strictly neccessary

To clarify, I intend on merging the stacked PRs together as one schema change. I will be sure to get the Go schema change deployed first once the target schema is finalized.

jeremywiebe

A few thoughts, but nothing blocking! Thanks Matt!

jeremywiebe · 2026-06-03T18:21:18Z

+                                {coord: [-8, -7]},
+                                {coord: [-8, -3]},
+                                {coord: [-4, -3]},


These item-data/ files should never be modified as they represent data imported from production. Instead, create a second file with the new structure(s).

See my comment at the top of this file.

Ah, thanks for the heads up! I missed the note at the top of the file. Do we need a regression test in this context, or is unit testing the parser enough?

You should add a new file with this new data format. That way we ensure that our parser remains compatible with all previous known schema versions and supports the new version.

jeremywiebe · 2026-06-03T18:46:47Z

-                [-1, 0],
-                [1, 0],
-            ],
+            points: [{coord: [0, 2]}, {coord: [-1, 0]}, {coord: [1, 0]}],


nit: I'm not sure if prettier chose this, but the original, multi-line layout reads more easily. No worries if this new inline layout if prettier's choice.

Prettier doesn't allow:

points: [ { coord: [1, 1] }, { coord: [2, 2] }, { coord: [3, 3] }, ],

but does allow:

points: [ { coord: [1, 1], }, { coord: [2, 2], }, { coord: [3, 3], }, ],

which mirrors the labels below, so I'll update it to that.

jeremywiebe · 2026-06-03T18:48:13Z

                        expect.objectContaining({
                            type: "polygon",
-                            points: [...defaultPolygon.points, [0, 0]],
+                            points: [...defaultPolygon.points, {coord: [0, 0]}],


I'm assuming this spread works because defaultPolygon.points has been changed to be an array of these new {coord: [number, number]} format?

Right! We do that in packages/perseus-core/src/utils/get-default-figure-for-type.ts

jeremywiebe · 2026-06-03T19:00:37Z

+                            points: squarePolygonPoints
+                                .slice(1)
+                                .map((coord) => ({coord})),


suggestion: This predates your change, but using the original test data in the assertion is a smell We should inline the values like we do later in this file around lines 1152-1156 (next chunk in this PR)

Agreed! The assertion has been updated.

jeremywiebe · 2026-06-03T19:03:10Z

+    const coords = points.map((point) => point.coord);
+    const coordsToPoints = (coords: Coord[]) =>
+        coords.map((coord) => ({coord}));


I haven't looked through all the usages of coords in this area, but I wonder if it'd be cleaner to use the new structure everywhere rather than have to keep dealing with both [number, number] and {x: number, y: number}.

Agreed! I updated the logic to always derive from the points prop

…ly in locked polygon settings

…ked polygon component

[feature/locked-polygon-point/1] refactor: locked polygon points shape

126165f

mojadem self-assigned this Jun 1, 2026

mojadem added the olc-5.0.f429d label Jun 1, 2026

mojadem requested a review from SonicScrewdriver June 1, 2026 20:56

github-actions Bot added the schema-change Attached to PRs when we detect Perseus Schema changes in it label Jun 1, 2026

github-actions Bot added the item-splitting-change label Jun 1, 2026

mojadem added 4 commits June 2, 2026 09:55

[mojadem/locked-polygon-point/1] docs(changeset): Change shape of `Lo…

3c5dcc8

…ckedPolygonType.points` from `[]Coord` to `LockedPolygonPointType`

[mojadem/locked-polygon-point/1] change points from LockedPolygonPoin…

e508266

…tType to []LockedPolygonPointType

[mojadem/locked-polygon-point/1] clean up parsing logic

3c11b2b

[mojadem/locked-polygon-point/1] add parser comment

c327e6b

mojadem added the item-splitting-change-ack Acknowledges that this PR's item-splitting bundle change has been reviewed label Jun 2, 2026

SonicScrewdriver approved these changes Jun 3, 2026

View reviewed changes

jeremywiebe approved these changes Jun 3, 2026

View reviewed changes

mojadem added 5 commits June 5, 2026 11:02

[mojadem/locked-polygon-point/1] derive logic from points prop direct…

2a6e73b

…ly in locked polygon settings

[mojadem/locked-polygon-point/1] revert item data

0d9c3c2

[mojadem/locked-polygon-point/1] update formatting

cf7875f

[mojadem/locked-polygon-point/1] use explicit data in assertion

b2acb96

[mojadem/locked-polygon-point/1] derive logic from points prop in loc…

815a548

…ked polygon component

mojadem mentioned this pull request Jun 5, 2026

feat: add showAngle field to LockedPolygonPointType #3730

Open

Conversation

mojadem commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Next steps

Test plan

Uh oh!

github-actions Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

npm Snapshot: Published

Uh oh!

github-actions Bot commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SonicScrewdriver left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeremywiebe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mojadem commented Jun 1, 2026 •

edited

Loading

github-actions Bot commented Jun 1, 2026 •

edited

Loading

github-actions Bot commented Jun 1, 2026 •

edited

Loading

SonicScrewdriver left a comment •

edited

Loading