Skip to content

refactor: locked polygon points shape #3705

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.

Sign up for GitHub

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

Open
mojadem wants to merge 10 commits into
base: main
Choose a base branch
from
Open

refactor: locked polygon points shape #3705

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
126165f
[feature/locked-polygon-point/1] refactor: locked polygon points shape
mojadem Jun 1, 2026
3c5dcc8
[mojadem/locked-polygon-point/1] docs(changeset): Change shape of `Lo…
mojadem Jun 2, 2026
e508266
[mojadem/locked-polygon-point/1] change points from LockedPolygonPoin…
mojadem Jun 2, 2026
3c11b2b
[mojadem/locked-polygon-point/1] clean up parsing logic
mojadem Jun 2, 2026
c327e6b
[mojadem/locked-polygon-point/1] add parser comment
mojadem Jun 2, 2026
2a6e73b
[mojadem/locked-polygon-point/1] derive logic from points prop direct…
mojadem Jun 5, 2026
0d9c3c2
[mojadem/locked-polygon-point/1] revert item data
mojadem Jun 5, 2026
cf7875f
[mojadem/locked-polygon-point/1] update formatting
mojadem Jun 5, 2026
b2acb96
[mojadem/locked-polygon-point/1] use explicit data in assertion
mojadem Jun 5, 2026
815a548
[mojadem/locked-polygon-point/1] derive logic from points prop in loc…
mojadem Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/hungry-masks-hang.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@khanacademy/perseus-core": major
"@khanacademy/perseus": minor
"@khanacademy/perseus-editor": minor
"@khanacademy/perseus-linter": minor
---

Change shape of `LockedPolygonType.points` from `[]Coord` to `LockedPolygonPointType`. This is to support adding fields to the Locked Polygon points in the future. Callers should migrate to using the new shape.
10 changes: 5 additions & 5 deletions __docs__/sample-data.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,11 +84,11 @@ export const graphExample: PerseusRenderer = {
weight: "thick",
fillStyle: "translucent",
points: [
[7, -3],
[8, -5],
[4, -7],
[0, -7],
[2, -3],
{coord: [7, -3]},
{coord: [8, -5]},
{coord: [4, -7]},
{coord: [0, -7]},
{coord: [2, -3]},
],
showVertices: false,
strokeStyle: "solid",
Expand Down
6 changes: 5 additions & 1 deletion packages/perseus-core/src/data-schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,9 +1075,13 @@ export type LockedEllipseType = {
ariaLabel?: string;
};

export type LockedPolygonPointType = {
coord: Coord;
};

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

@SonicScrewdriver SonicScrewdriver Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mojadem mojadem Jun 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

color: LockedFigureColor;
showVertices: boolean;
fillStyle: LockedFigureFillType;
Expand Down
174 changes: 174 additions & 0 deletions ...ages/perseus-core/src/parse-perseus-json/perseus-parsers/interactive-graph-widget.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,180 @@ describe("parseInteractiveGraphWidget", () => {
);
});

it("parses locked polygon points with coord field", () => {
const result = parse(
{
type: "interactive-graph",
options: {
step: [1, 1],
markings: "grid",
showProtractor: false,
range: [
[-10, 10],
[-10, 10],
],
showAxisArrows: {
xMin: true,
xMax: true,
yMin: true,
yMax: true,
},
showAxisTicks: {x: true, y: true},
correct: {
type: "linear",
},
lockedFigures: [
{
type: "polygon",
points: [
{coord: [0, 0]},
{coord: [1, 0]},
{coord: [1, 1]},
],
color: "blue",
showVertices: false,
fillStyle: "none",
strokeStyle: "solid",
weight: "medium",
},
],
},
},
parseInteractiveGraphWidget,
);

expect(result).toEqual(
success({
type: "interactive-graph",
options: {
step: [1, 1],
markings: "grid",
showProtractor: false,
range: [
[-10, 10],
[-10, 10],
],
showAxisArrows: {
xMin: true,
xMax: true,
yMin: true,
yMax: true,
},
showAxisTicks: {x: true, y: true},
correct: {
type: "linear",
},
graph: {
type: "linear",
},
lockedFigures: [
{
type: "polygon",
points: [
{coord: [0, 0]},
{coord: [1, 0]},
{coord: [1, 1]},
],
color: "blue",
showVertices: false,
fillStyle: "none",
strokeStyle: "solid",
weight: "medium",
labels: [],
},
],
},
}),
);
});

it("normalizes legacy locked polygon points array to coord field", () => {
const result = parse(
{
type: "interactive-graph",
options: {
step: [1, 1],
markings: "grid",
showProtractor: false,
range: [
[-10, 10],
[-10, 10],
],
showAxisArrows: {
xMin: true,
xMax: true,
yMin: true,
yMax: true,
},
showAxisTicks: {x: true, y: true},
correct: {
type: "linear",
},
lockedFigures: [
{
type: "polygon",
points: [
[0, 0],
[1, 0],
[1, 1],
],
color: "blue",
showVertices: false,
fillStyle: "none",
strokeStyle: "solid",
weight: "medium",
},
],
},
},
parseInteractiveGraphWidget,
);

expect(result).toEqual(
success({
type: "interactive-graph",
options: {
step: [1, 1],
markings: "grid",
showProtractor: false,
range: [
[-10, 10],
[-10, 10],
],
showAxisArrows: {
xMin: true,
xMax: true,
yMin: true,
yMax: true,
},
showAxisTicks: {x: true, y: true},
correct: {
type: "linear",
},
graph: {
type: "linear",
},
lockedFigures: [
{
type: "polygon",
points: [
{coord: [0, 0]},
{coord: [1, 0]},
{coord: [1, 1]},
],
color: "blue",
showVertices: false,
fillStyle: "none",
strokeStyle: "solid",
weight: "medium",
labels: [],
},
],
},
}),
);
});

it("rejects unrecognized color names on locked figures", () => {
const result = parse(
{
Expand Down
16 changes: 15 additions & 1 deletion packages/perseus-core/src/parse-perseus-json/perseus-parsers/interactive-graph-widget.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,9 +256,23 @@ const parseLockedEllipseType = object({
ariaLabel: optional(string),
});

const parseLockedPolygonPointType = object({
coord: pairOfNumbers,
});

const parseLockedPolygonPointsType = union(
array(parseLockedPolygonPointType),
).or(
// Normalize legacy representation of points as Coord[] to
// LockedPolygonPointType[].
pipeParsers(array(pairOfNumbers)).then(
convert((coords) => coords.map((coord) => ({coord}))),
).parser,
).parser;

const parseLockedPolygonType = object({
type: constant("polygon"),
points: array(pairOfNumbers),
points: parseLockedPolygonPointsType,
color: parseLockedFigureColor,
showVertices: boolean,
fillStyle: parseLockedFigureFillType,
Expand Down
60 changes: 36 additions & 24 deletions ...se-perseus-json/regression-tests/__snapshots__/parse-perseus-json-regression.test.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6972,18 +6972,24 @@ exports[`parseAndMigratePerseusItem given interactive-graph-locked-figures-missi
"fillStyle": "none",
"labels": [],
"points": [
[
-8,
-7,
],
[
-8,
-3,
],
[
-4,
-3,
],
{
"coord": [
-8,
-7,
],
},
{
"coord": [
-8,
-3,
],
},
{
"coord": [
-4,
-3,
],
},
],
"showVertices": false,
"strokeStyle": "solid",
Expand Down Expand Up @@ -7248,18 +7254,24 @@ exports[`parseAndMigratePerseusItem given interactive-graph-locked-figures-missi
"fillStyle": "none",
"labels": [],
"points": [
[
-8,
-7,
],
[
-8,
-3,
],
[
-4,
-3,
],
{
"coord": [
-8,
-7,
],
},
{
"coord": [
-8,
-3,
],
},
{
"coord": [
-4,
-3,
],
},
],
"showVertices": false,
"strokeStyle": "solid",
Expand Down
6 changes: 3 additions & 3 deletions ...rseus-json/regression-tests/item-data/interactive-graph-locked-figures-missing-weights.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,9 @@ export default {
{
type: "polygon",
points: [
[-8, -7],
[-8, -3],
[-4, -3],
{coord: [-8, -7]},
{coord: [-8, -3]},
{coord: [-4, -3]},

@jeremywiebe jeremywiebe Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mojadem mojadem Jun 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jeremywiebe jeremywiebe Jun 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
color: "pink",
showVertices: false,
Expand Down
18 changes: 3 additions & 15 deletions packages/perseus-core/src/utils/generators/interactive-graph-widget-generator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,11 +1179,7 @@ describe("generateIGLockedPolygon", () => {
// Assert
expect(lockedPolygon).toEqual({
type: "polygon",
points: [
[0, 2],
[-1, 0],
[1, 0],
],
points: [{coord: [0, 2]}, {coord: [-1, 0]}, {coord: [1, 0]}],

@jeremywiebe jeremywiebe Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@mojadem mojadem Jun 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

color: "grayH",
showVertices: false,
fillStyle: "none",
Expand All @@ -1196,11 +1192,7 @@ describe("generateIGLockedPolygon", () => {
it("builds a locked polygon with all props", () => {
// Arrange, Act
const lockedPolygon = generateIGLockedPolygon({
points: [
[1, 1],
[2, 2],
[3, 3],
],
points: [{coord: [1, 1]}, {coord: [2, 2]}, {coord: [3, 3]}],
color: "blue",
showVertices: true,
fillStyle: "solid",
Expand All @@ -1221,11 +1213,7 @@ describe("generateIGLockedPolygon", () => {
// Assert
expect(lockedPolygon).toEqual({
type: "polygon",
points: [
[1, 1],
[2, 2],
[3, 3],
],
points: [{coord: [1, 1]}, {coord: [2, 2]}, {coord: [3, 3]}],
color: "blue",
showVertices: true,
fillStyle: "solid",
Expand Down
6 changes: 1 addition & 5 deletions packages/perseus-core/src/utils/get-default-figure-for-type.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,7 @@ describe("getDefaultFigureForType", () => {
const figure = getDefaultFigureForType("polygon");
expect(figure).toEqual({
type: "polygon",
points: [
[0, 2],
[-1, 0],
[1, 0],
],
points: [{coord: [0, 2]}, {coord: [-1, 0]}, {coord: [1, 0]}],
color: "grayH",
showVertices: false,
fillStyle: "none",
Expand Down
6 changes: 1 addition & 5 deletions packages/perseus-core/src/utils/get-default-figure-for-type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,7 @@ export function getDefaultFigureForType(type: LockedFigureType): LockedFigure {
case "polygon":
return {
type: "polygon",
points: [
[0, 2],
[-1, 0],
[1, 0],
],
points: [{coord: [0, 2]}, {coord: [-1, 0]}, {coord: [1, 0]}],
color: DEFAULT_COLOR,
showVertices: false,
fillStyle: "none",
Expand Down
Loading
Loading