[i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. (#2264) #2264

SonicScrewdriver · 2025-02-22T00:55:22Z

Summary:

This PR is part of the Interactive Graph Project!

The work in this PR ensures that users can move points across invalid locations for all graph types. This required updates to the following Interactive Graph types:

Quadratic
Sinsoid
Circle
All Graphs that use Movable Line
- Segment
- Linear
- Linear System
- Ray

NOTE: This PR does not change the functionality of the Polygon or Angle Graphs as I believe both have custom implementations that have already been implemented.

Issue: LEMS-2014

Video:

Screen.Recording.2025-02-21.at.5.20.38.PM.mov

Storybook Link:

https://650db21c3f5d1b2f13c02952-ehxlgsxqwf.chromatic.com/

Issue: LEMS-2014

Test plan:

All current tests pass
New tests
Manual testing in Storybook

Author: SonicScrewdriver

Reviewers: SonicScrewdriver, nishasy, catandthemachines

Required Reviewers:

Approved By: nishasy

Checks: ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2264

…d Quadratic graph points past invalid locations.

…ns for all graphs + new tests

github-actions · 2025-02-22T00:57:16Z

Size Change: -45 B (-0.01%)

Total Size: 872 kB

Filename	Size	Change
`packages/perseus-core/dist/es/index.js`	29.8 kB	-45 B (-0.15%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	39.5 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-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/index.js`	367 kB
`packages/perseus/dist/es/strings.js`	6.57 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-22T00:57:30Z

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2264

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

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

SonicScrewdriver · 2025-02-22T00:58:11Z

packages/perseus/src/widgets/interactive-graphs/graphs/quadratic.test.tsx

@@ -328,3 +295,74 @@ describe("describedQuadraticGraph interactive elements", () => {
        );
    });
 });
+
+describe("getQuadraticCoefficients", () => {


I renamed this as it was only testing the getQuadraticCoefficients function.

Thank you for this comment! I'm confused why it can't figure out the body of this is the same as before.

…ove points across invalid locations for all graphs.

SonicScrewdriver · 2025-02-22T01:13:10Z

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

+} => {
+    // Create a helper function that moves the point and then checks
+    // if it overlaps with the center point after the move.
+    const movePointWithConstraint = (


This function is recreated in a few places, albeit with several differences to support each graph type. I ruminated on whether to move this to a util file, but I think the subtle differences and value of having this logic "in situ" outweigh what we would gain by keeping this more DRY.

I'm open to other interpretations on this, however!

I could go either way between your current implementation and a util function that you pass in the type to so you can easily see what the differences are. Since you've already written it this way, I say leave it as is.

SonicScrewdriver · 2025-02-22T01:30:54Z

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

@@ -768,6 +766,195 @@ describe("MafsGraph", () => {
        expect(state.coords).toEqual(expectedCoords);
    });

+    it("Quadratic moves points over invalid locations on keystroke", async () => {


I wanted to write these tests in their respective graph testing files, but I was unable to get the graph to update properly in jest after keyboard interaction.

Given that mafs-graphs.test.tsx was already testing some keyboard functionality, and had the necessary logic for accessing the state directly, I've opted to implement the keyboard tests in this file.

Happy to take any feedback or suggestions on this as well!

SonicScrewdriver · 2025-02-22T01:33:58Z

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

@@ -150,10 +150,7 @@ function doFocusPoint(
    switch (state.type) {


Hmm it seems like my settings resulted in removing some white space from this file. I will revert that as there are no other changes.

… Graph reducer.

nishasy

Nice! This works BEAUTIFULLY in storybook!

nishasy · 2025-02-24T19:14:54Z

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

+} => {
+    // Create a helper function that moves the point and then checks
+    // if it overlaps with the center point after the move.
+    const movePointWithConstraint = (


I could go either way between your current implementation and a util function that you pass in the type to so you can easily see what the differences are. Since you've already written it this way, I say leave it as is.

nishasy · 2025-02-24T19:17:52Z

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

+        // If the moved point overlaps with the center point,
+        // move the point again.
+        if (vec.dist(movedCoord, center) === 0) {
+            movedCoord = moveFunc(movedCoord);


nishasy · 2025-02-24T19:20:01Z

packages/perseus/src/widgets/interactive-graphs/graphs/components/movable-line.tsx

@@ -240,6 +252,54 @@ const Line = (props: LineProps) => {
    );
 };

+export const getMovableLineKeyboardConstraint = (


I love that this fixes the ray overlapping points issue!

(It still happens with mouse, but that is irrelevant here and arguably not an issue.)

nishasy · 2025-02-24T19:23:23Z

packages/perseus/src/widgets/interactive-graphs/graphs/quadratic.test.tsx

@@ -328,3 +295,74 @@ describe("describedQuadraticGraph interactive elements", () => {
        );
    });
 });
+
+describe("getQuadraticCoefficients", () => {


Thank you for this comment! I'm confused why it can't figure out the body of this is the same as before.

SonicScrewdriver added 2 commits February 20, 2025 17:06

[i-like-to-move-it] Ensure that keyboard users can move Sinusoidal an…

63ea593

…d Quadratic graph points past invalid locations.

[i-like-to-move-it] Ensure points can be moved across invalid locatio…

93a6388

…ns for all graphs + new tests

SonicScrewdriver self-assigned this Feb 22, 2025

SonicScrewdriver commented Feb 22, 2025

View reviewed changes

[i-like-to-move-it] Update comments

6ed02a2

SonicScrewdriver changed the title ~~[i-like-to-move-it] Ensure that keyboard users can move Sinusoidal and Quadratic graph points past invalid locations.~~ [i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. Feb 22, 2025

[i-like-to-move-it] docs(changeset): Ensure that keyboard users can m…

e7e4a60

…ove points across invalid locations for all graphs.

SonicScrewdriver commented Feb 22, 2025

View reviewed changes

[i-like-to-move-it] Minor comment improvement for clarity

d95e453

SonicScrewdriver requested review from nishasy and catandthemachines February 22, 2025 01:23

SonicScrewdriver commented Feb 22, 2025

View reviewed changes

[i-like-to-move-it] Undo silly white space changes to the Interactive…

e9bf133

… Graph reducer.

nishasy approved these changes Feb 24, 2025

View reviewed changes

SonicScrewdriver changed the title ~~[i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs.~~ [i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. (#2264) Feb 24, 2025

SonicScrewdriver merged commit 4eb9fe0 into main Feb 24, 2025
8 checks passed

SonicScrewdriver deleted the i-like-to-move-it branch February 24, 2025 21:01

khan-actions-bot mentioned this pull request Feb 24, 2025

Version Packages #2258

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. (#2264) #2264

[i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. (#2264) #2264

SonicScrewdriver commented Feb 22, 2025 •

edited

Loading

github-actions bot commented Feb 22, 2025 •

edited

Loading

github-actions bot commented Feb 22, 2025 •

edited

Loading

SonicScrewdriver Feb 22, 2025

nishasy Feb 24, 2025

SonicScrewdriver Feb 22, 2025

nishasy Feb 24, 2025

SonicScrewdriver Feb 22, 2025

SonicScrewdriver Feb 22, 2025

nishasy left a comment

nishasy Feb 24, 2025

nishasy Feb 24, 2025

nishasy Feb 24, 2025

nishasy Feb 24, 2025

		@@ -150,10 +150,7 @@ function doFocusPoint(
		switch (state.type) {

[i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. (#2264) #2264

[i-like-to-move-it] Ensure that keyboard users can move points across invalid locations for all graphs. (#2264) #2264

Conversation

SonicScrewdriver commented Feb 22, 2025 • edited Loading

Summary:

Video:

Storybook Link:

Test plan:

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

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

npm Snapshot: Published

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

nishasy 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

SonicScrewdriver commented Feb 22, 2025 •

edited

Loading

github-actions bot commented Feb 22, 2025 •

edited

Loading

github-actions bot commented Feb 22, 2025 •

edited

Loading