[Interactive Graph] Polygon side snapping is keyboard accessible. #2270

catandthemachines · 2025-02-25T23:55:25Z

Summary:

Updating Polygon interactive graph implementation to have keyboard accessible side snapping. This is done by hooking up the side snapping behavior into a constraint function to make to keyboard arrow keys.

Issue: LEMS-2243

Test plan:

Go to: /?path=/story/perseuseditor-widgets-interactive-graph--interactive-graph-polygon
Set Snap to: to side measures
Use your keyboard (tab and arrow keys) to move the various polygon points.
Notice they now move!

…ing.

…lementation.

…and mouse behavior.

… we keep it whole numbers.

github-actions · 2025-02-25T23:57:14Z

Size Change: +1.34 kB (+0.15%)

Total Size: 874 kB

Filename	Size	Change
`packages/perseus/dist/es/index.js`	369 kB	+1.34 kB (+0.36%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39.7 kB
`packages/keypad-context/dist/es/index.js`	760 B
`packages/kmath/dist/es/index.js`	11.1 kB
`packages/math-input/dist/es/index.js`	78.2 kB
`packages/math-input/dist/es/strings.js`	1.79 kB
`packages/perseus-core/dist/es/index.js`	29.8 kB
`packages/perseus-editor/dist/es/index.js`	276 kB
`packages/perseus-linter/dist/es/index.js`	22.8 kB
`packages/perseus-score/dist/es/index.js`	20.6 kB
`packages/perseus/dist/es/strings.js`	6.74 kB
`packages/pure-markdown/dist/es/index.js`	4.14 kB
`packages/simple-markdown/dist/es/index.js`	13.1 kB

_{compressed-size-action}

github-actions · 2025-02-25T23:57:33Z

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2270

If you are working in Khan Academy's webapp, you can run:

./dev/tools/bump_perseus_version.sh -t PR2270

…olygons to be keyboard accessible.

nishasy · 2025-02-27T00:19:43Z

packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

-    const constrain = ["angles", "sides"].includes(snapTo)
-        ? (p) => p
-        : (p) => snap(snapStep, p);
+    const constrain: KeyboardMovementConstraint =


Nit: Can this be a function named getConstraintFn that includes a switch statement for the snapTo type? I think that might be easier to follow for engineers looking at this in the future.

Something like

function getConstraintFn(): KeyboardMovementConstraint { switch(snapTo) { case "angles": return p => p case "sides": return getSideSnapConstraint(...) default: // "grid" return p => snap(snapStep, p); } }

And then you can pass just this function into constrain below.

nishasy · 2025-02-27T00:25:19Z

packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

@@ -683,3 +689,60 @@ function describePolygonGraph(
        srPolygonInteractiveElements,
    };
 }
+
+export function getSideSnapConstraint(


Is there a test for this?

nishasy · 2025-02-27T00:25:26Z

packages/perseus/src/widgets/interactive-graphs/utils.ts

@@ -66,6 +66,22 @@ export function bound({
    return clampToBox(boundingBox, point);
 }

+// Returns true if the point is within the range of the graph.
+export function isInBound({


Is there a test for this?

nishasy · 2025-02-27T00:28:14Z

Question: I see this is allowing approximate sides too. Is that intended?

catandthemachines · 2025-02-27T00:35:57Z

Question: I see this is allowing approximate sides too. Is that intended?

Yes, this is how the functionality works currently. It will show that when it's not exactly a whole number, but it's very close.

benchristel

This looks good overall! Thanks for this huge accessibility improvement!

Requesting changes because coords is being mutated in the reducer; we should make a copy instead. I left a few other suggestions inline too.

benchristel · 2025-02-26T23:09:40Z

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts

    // Takes the destination point and makes sure it is within the bounds of the graph
    // SnapStep is [0, 0] because we don't want to snap to the grid
-    coordsCopy[index] = bound({
+    coords[index] = bound({


This mutates the input coords array, which is likely to cause problems at some point because React assumes immutability. We should use setAtIndex to immutably transform the array.

const targetCoords = setAtIndex({ array: coords, index, newValue: bound({point: destinationPoint, range, snapStep: [0, 0]}), });

I adjusted the implementation to have the components/functions that call this one to pass in a coordsCopy, so there's no issue about mutating the shared state. Though as I'm looking at it this might cause issues down the line, I'll adjust this to make sure the function make's it's own copy so other functions just need to pass in the coord state. 😉

benchristel · 2025-02-26T23:15:40Z

packages/perseus/src/widgets/interactive-graphs/utils.ts

+    return (
+        point[0] >= range[0][0] &&
+        point[0] <= range[0][1] &&
+        point[1] >= range[1][0] &&
+        point[1] <= range[1][1]
+    );


FWIW we have constants in interactive-graphs/math/interval.ts and interactive-graphs/math/coordinates.ts that could potentially make this easier to read:

return ( point[X] >= range[X][MIN] && point[X] <= range[X][MAX] && point[Y] >= range[Y][MIN] && point[Y] <= range[Y][MAX] );

Though I don't think we use these constants in many places, so maybe they're more surprising than helpful.

It also might be worth moving this function to interactive-graphs/math/box.ts since that is the module that knows all about ranges and points.

That sounds good! One question I have is if we have a function/utility that already check this? I know we have the bound function that returns a point within the bounds of our graph, but I could not find a function that returns true or false if a point is outside the graph. 🤔

benchristel · 2025-02-27T15:34:22Z

packages/perseus/src/widgets/interactive-graphs/graphs/polygon.tsx

-        ? (p) => p
-        : (p) => snap(snapStep, p);
+    const constrain: KeyboardMovementConstraint =
+        snapTo === "angles" ? (p) => p : (p) => snap(snapStep, p);


Could we use a switch statement to get the constrain value? Currently, the logic is split between two ternaries that are a couple hundred lines apart, and I think that will confuse future readers.

function getKeyboardMovementConstraintForPoint( pointIndex: number, ): KeyboardMovementConstraint { switch (snapTo) { case "grid": return (p) => snap(snapStep, p); case "angles": return (p) => p; case "sides": return getSideSnapConstraint(points, pointIndex, graphConfig.range); default: throw new UnreachableCaseError(snapTo); } }

Complication: it looks like the constrain on line 102 is used in the useDraggable below, which I think is for moving the whole polygon. So we'd also need:

function getConstrainFunctionForWholePolygon(): (point: vec.Vector2) => vec.Vector2 { return snapTo === "angles" ? (p) => p : (p) => snap(snapStep, p); }

catandthemachines · 2025-02-26T00:55:00Z

packages/perseus/src/widgets/interactive-graphs/utils.ts

@@ -66,6 +66,22 @@ export function bound({
    return clampToBox(boundingBox, point);
 }

+// Returns true if the point is within the range of the graph.
+export function isInBound({


Is there another function that currently exists to check if a point is in bounds? I know we have the function bound which moves a point to be in bounds, but it doesn't seem like we have one as a simple boolean.

catandthemachines · 2025-02-26T00:56:29Z

packages/perseus/src/widgets/interactive-graphs/interactive-graph.testdata.ts

I really want these test examples to be on graph markings. It makes it easier to debug and troubleshoot. ❤️

catandthemachines · 2025-02-26T00:57:14Z

packages/perseus/src/widgets/interactive-graphs/__snapshots__/interactive-graph.test.tsx.snap

I'm not sure why this file changed. I'm going to take a look and make sure this is correct.

catandthemachines · 2025-02-27T22:09:08Z

packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts

    // Takes the destination point and makes sure it is within the bounds of the graph
    // SnapStep is [0, 0] because we don't want to snap to the grid
-    coordsCopy[index] = bound({
+    coords[index] = bound({


I adjusted the implementation to have the components/functions that call this one to pass in a coordsCopy, so there's no issue about mutating the shared state. Though as I'm looking at it this might cause issues down the line, I'll adjust this to make sure the function make's it's own copy so other functions just need to pass in the coord state. 😉

catandthemachines · 2025-02-27T22:13:10Z

packages/perseus/src/widgets/interactive-graphs/utils.ts

+    return (
+        point[0] >= range[0][0] &&
+        point[0] <= range[0][1] &&
+        point[1] >= range[1][0] &&
+        point[1] <= range[1][1]
+    );


That sounds good! One question I have is if we have a function/utility that already check this? I know we have the bound function that returns a point within the bounds of our graph, but I could not find a function that returns true or false if a point is outside the graph. 🤔

catandthemachines added 10 commits February 19, 2025 17:43

[catjohnson/lems-2243] initial idea for angle and side snapping track…

326d7bc

…ing.

[catjohnson/lems-2243] Initial prototyping of the snapping fix.

4fd11c4

[catjohnson/lems-2243] fixing linting.

5980c15

[catjohnson/lems-2243] more linting.

1e83c41

[catjohnson/lems-2243] Moving keyboard functionality to polygon.

cb22e21

[catjohnson/lems-2243] adding back the side snapping in the mouse imp…

b09d0b1

…lementation.

[catjohnson/lems-2243] Removing unnecessary file

0eab122

[catjohnson/lems-2243] refactoring out repeat logic used by keyboard …

036ef07

…and mouse behavior.

[catjohnson/lems-2243] Showing what the side snapping looks like when…

5907617

… we keep it whole numbers.

[catjohnson/lems-2243] removing snapstep adjustment.

00b445d

catandthemachines self-assigned this Feb 25, 2025

catandthemachines added 7 commits February 25, 2025 16:01

[catjohnson/lems-2243] revert changes.

f34f591

[catjohnson/lems-2243] fixing angle snapping.

86c0bb2

[catjohnson/lems-2243] cleaning up conditions.

2e9658a

[catjohnson/lems-2243] Updating comment

345beba

[catjohnson/lems-2243] Merge branch 'main' into catjohnson/lems-2243

1abdc9d

[catjohnson/lems-2243] docs(changeset): Fixing side snapping within p…

0dbdc7f

…olygons to be keyboard accessible.

[catjohnson/lems-2243] updating snapshots.

dc422cf

catandthemachines requested review from nishasy, SonicScrewdriver and benchristel February 26, 2025 00:57

nishasy reviewed Feb 27, 2025

View reviewed changes

benchristel requested changes Feb 27, 2025

View reviewed changes

[catjohnson/lems-2243] Merge branch 'main' into catjohnson/lems-2243

5bdf5dd

catandthemachines commented Feb 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Interactive Graph] Polygon side snapping is keyboard accessible. #2270

[Interactive Graph] Polygon side snapping is keyboard accessible. #2270

catandthemachines commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

nishasy Feb 27, 2025 •

edited

Loading

nishasy Feb 27, 2025

nishasy Feb 27, 2025

nishasy commented Feb 27, 2025

catandthemachines commented Feb 27, 2025

benchristel left a comment

benchristel Feb 26, 2025

catandthemachines Feb 27, 2025

benchristel Feb 26, 2025

catandthemachines Feb 27, 2025

benchristel Feb 27, 2025

catandthemachines Feb 26, 2025

catandthemachines Feb 26, 2025

catandthemachines Feb 26, 2025

catandthemachines Feb 27, 2025

catandthemachines Feb 27, 2025

[Interactive Graph] Polygon side snapping is keyboard accessible. #2270

Are you sure you want to change the base?

[Interactive Graph] Polygon side snapping is keyboard accessible. #2270

Conversation

catandthemachines commented Feb 25, 2025 • edited Loading

Summary:

Test plan:

github-actions bot commented Feb 25, 2025 • edited Loading

github-actions bot commented Feb 25, 2025 • edited Loading

npm Snapshot: Published

nishasy Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishasy commented Feb 27, 2025

catandthemachines commented Feb 27, 2025

benchristel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catandthemachines commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

nishasy Feb 27, 2025 •

edited

Loading