-
Notifications
You must be signed in to change notification settings - Fork 351
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
Conversation
…behavior for keyboard users in polygon examples.
Size Change: +281 B (+0.03%) Total Size: 875 kB
ℹ️ View Unchanged
|
npm Snapshot: PublishedGood news!! We've packaged up the latest commit from this PR (f7654a1) and published it to npm. You 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
- 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.
- 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
packages/perseus/src/widgets/interactive-graphs/reducer/interactive-graph-reducer.ts
Show resolved
Hide resolved
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 |
…till worries this issue will continue to happen in polygon though...
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? |
There was a problem hiding this 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.
packages/perseus/src/widgets/interactive-graphs/graphs/components/angle-indicators.tsx
Outdated
Show resolved
Hide resolved
packages/perseus/src/widgets/interactive-graphs/graphs/utils.ts
Outdated
Show resolved
Hide resolved
Bug to fix the weird exploding: https://khanacademy.atlassian.net/browse/LEMS-2901 |
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: