Skip to content
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

[Interactive Graph] Improve Polygon angle snapping is for keyboard users. #2281

Merged
merged 16 commits into from
Mar 6, 2025

Conversation

catandthemachines
Copy link
Member

Summary:

Updating Polygon interactive graph implementation to have a better keyboard accessible angle snapping. This is done by hooking up the angle snapping behavior into a constraint function to make the keyboard arrow keys more effective in locating the next best point and angle combination.

Issue: LEMS-2893

Test plan:

  • Go to: /?path=/story/perseuseditor-widgets-interactive-graph--interactive-graph-polygon
  • Set Snap to: to angle measures
  • Use your keyboard (tab and arrow keys) to move the various polygon points.
  • Notice the movement is smaller to have a better chance of getting the angles correct!

@catandthemachines catandthemachines self-assigned this Mar 4, 2025
…behavior for keyboard users in polygon examples.
Copy link
Contributor

github-actions bot commented Mar 4, 2025

Size Change: +281 B (+0.03%)

Total Size: 875 kB

Filename Size Change
packages/perseus/dist/es/index.js 369 kB +281 B (+0.08%)
ℹ️ 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 77.7 kB
packages/math-input/dist/es/strings.js 1.79 kB
packages/perseus-core/dist/es/index.js 31.9 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.73 kB
packages/pure-markdown/dist/es/index.js 4.14 kB
packages/simple-markdown/dist/es/index.js 13.1 kB

compressed-size-action

Copy link
Contributor

github-actions bot commented Mar 4, 2025

npm Snapshot: Published

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

Example:

pnpm add @khanacademy/perseus@PR2281

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Per discussion in our previous PR, I am removing these .withMarkings("none") type as it's unnecessary for testing and make it harder to debug for developers.

#2270 (comment)

Copy link
Contributor

@nishasy nishasy left a comment

Choose a reason for hiding this comment

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

When trying stuff out in this PR's storybook publish, I observed 2 supposed bugs.

  1. I held down the arrow key on a point with interior angle snapping and eventually the whole graph crashed. I'm having a hard time reproducing it now though.

Screenshot 2025-03-04 at 5 19 42 PM

  1. It looks like the point gets stuck on the edges of the graph when snapping to interior angles.
Screen.Recording.2025-03-04.at.5.21.12.PM.mov

@nishasy
Copy link
Contributor

nishasy commented Mar 6, 2025

@catandthemachines

I held down the arrow key on a point with interior angle snapping and eventually the whole graph crashed. I'm having a hard time reproducing it now though.

Never mind, I can repro it consistently now using the configuration in this video.

Screen.Recording.2025-03-05.at.4.22.36.PM.mov

@catandthemachines
Copy link
Member Author

@catandthemachines

I held down the arrow key on a point with interior angle snapping and eventually the whole graph crashed. I'm having a hard time reproducing it now though.

Never mind, I can repro it consistently now using the configuration in this video.

Screen.Recording.2025-03-05.at.4.22.36.PM.mov

Okay I think I resolved the issue? I'll still file the bug that we saw just in case with some context but hopefully it's gone?

Copy link
Contributor

@nishasy nishasy left a comment

Choose a reason for hiding this comment

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

While manually testing in the current storybook publish, I confirmed that the NaN and the getting stuck on the border bugs are gone!

Unfortunately, it did explode on me again :( But it's so hard to repro and it seems like a pretty weird and rare edge case. I'm in favor of filing a bug to come back to it later.

@catandthemachines
Copy link
Member Author

Bug to fix the weird exploding: https://khanacademy.atlassian.net/browse/LEMS-2901

@catandthemachines catandthemachines merged commit 015aace into main Mar 6, 2025
8 checks passed
@catandthemachines catandthemachines deleted the catjohnson/lems-2893 branch March 6, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants