Copy the implementation of `useMovable` into `useDraggable` #1370

benchristel · 2024-06-21T19:23:12Z

Summary:

This copies Mafs' implementation of the useMovable hook into our own useDraggable
hook with no substantive changes. In a future PR, I'll add the new features I need.

Context: We need the useMovable hook to do a couple things that Mafs doesn't
yet enable:

let us handle keyboard events explicitly (needed for angle graphs, and
maybe polygons if we decide to continue supporting side-length
snapping). Currently, Mafs uses heuristics and search to convert
keyboard input into movement vectors based on the provided constrain
function, but this only works for grid-based snapping.
get the movement vector in pixel coordinates. This is needed to
implement the protractor rotation handle. The protractor is a
fixed-size image that doesn't scale with the graph, so we really want
to be working in pixels - otherwise, we have to do a bunch of
confusing coordinate transformations.

We'll likely want to iterate on these features a bit before contributing
them back to Mafs. Therefore, this commit creates a wrapper around
useMovable where we can add our new functionality.

Issue: none

Test plan:

Run yarn dev and view the graphs at http://localhost:5173/.
Turn on the Mafs flags for all graph types.
Verify that you can move all the interactive graph elements with
mouse and keyboard.

khan-actions-bot · 2024-06-21T19:23:27Z

Gerald

Required Reviewers

@Khan/perseus for changes to .changeset/olive-rivers-eat.md, packages/perseus/package.json, packages/perseus/src/widgets/interactive-graphs/graphs/use-draggable.test.tsx, packages/perseus/src/widgets/interactive-graphs/graphs/use-draggable.ts

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

github-actions · 2024-06-21T19:24:56Z

npm Snapshot: Published

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

Example:

yarn add @khanacademy/perseus@PR1370

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

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

benchristel · 2024-06-21T19:28:21Z

packages/perseus/package.json

@use-gesture/react is a dependency of Mafs that we are now importing directly. I verified that adding it here doesn't cause multiple copies/versions of @use-gesture/react to get installed.

benchristel · 2024-06-21T19:30:17Z

packages/perseus/src/widgets/interactive-graphs/graphs/use-draggable.test.tsx

These tests are unfortunately incomplete. I couldn't figure out how to get @use-gesture/react to register a drag gesture, either with userEvent or fireEvent.

Suggestion: Is this Moving a Pointer section of the Testing Library docs useful? It seems like it is showing how to user userEvent.pointer to drag, though I could be reading it wrong.

Yeah, I tried what those docs suggest. @use-gesture/react doesn't see it as a drag, though.

Update: I looked at the tests for @use-gesture/react itself and found that they're using fireEvent, so I came up with a testing solution based on that.

github-actions · 2024-06-21T19:30:28Z

Size Change: +496 B (+0.06%)

Total Size: 847 kB

Filename	Size	Change
`packages/perseus-editor/dist/es/index.js`	272 kB	-1 B (0%)
`packages/perseus/dist/es/index.js`	408 kB	+497 B (+0.12%)

ℹ️ View Unchanged

Filename	Size
`packages/kas/dist/es/index.js`	38.2 kB
`packages/kmath/dist/es/index.js`	4.26 kB
`packages/math-input/dist/es/index.js`	80.1 kB
`packages/math-input/dist/es/strings.js`	1.73 kB
`packages/perseus-core/dist/es/index.js`	906 B
`packages/perseus-error/dist/es/index.js`	866 B
`packages/perseus-linter/dist/es/index.js`	21.8 kB
`packages/perseus/dist/es/strings.js`	3.21 kB
`packages/pure-markdown/dist/es/index.js`	3.67 kB
`packages/simple-markdown/dist/es/index.js`	12.4 kB

_{compressed-size-action}

codecov · 2024-06-21T19:32:42Z

Codecov Report

Attention: Patch coverage is 96.20253% with 6 lines in your changes missing coverage. Please review.

Project coverage is 71.35%. Comparing base (e5a54d8) to head (652c5b8).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1370      +/-   ##
==========================================
+ Coverage   69.63%   71.35%   +1.71%     
==========================================
  Files         492      495       +3     
  Lines      103871   104092     +221     
  Branches     7451    11277    +3826     
==========================================
+ Hits        72333    74270    +1937     
+ Misses      31356    29822    -1534     
+ Partials      182        0     -182

Files	Coverage Δ
...widgets/interactive-graphs/graphs/use-draggable.ts	`96.51% <96.20%> (-3.49%)`	⬇️

... and 146 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5a54d8...652c5b8. Read the comment docs.

Myranae

Looks good! Was able to test that all the Mafs graphs (or everything I could tab to) moved with keyboard and also with mouse. I was surprised to see the points on a polygon with angle snapping moving with keyboard, but the points on the polygon with sides snapping still don't move with the keyboard. Added some non-blocking comments.

Myranae · 2024-06-21T20:12:58Z

packages/perseus/src/widgets/interactive-graphs/graphs/use-draggable.test.tsx

+        <p role="button" ref={gestureTarget}>
+            dragging: {String(dragging)}
+        </p>


Question: Is it okay to set the role of a p element to "button" like this? I know this is only a test, so maybe it's not super important.

It's valid, but potentially confusing. I should probably just make this a button element.

Myranae · 2024-06-21T20:18:25Z

packages/perseus/src/widgets/interactive-graphs/graphs/use-draggable.test.tsx

Suggestion: Is this Moving a Pointer section of the Testing Library docs useful? It seems like it is showing how to user userEvent.pointer to drag, though I could be reading it wrong.

catandthemachines

Approved! Though I am curious why it is necessary to move the logic in Mafs into a useDraggable function for our internal use, instead of utilizing useMovable.

benchristel · 2024-06-26T17:48:23Z

@catandthemachines I've updated the PR description explaining the reasoning behind this change. Copied from here: #1367

@benchristel

This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @khanacademy/perseus@24.1.0 ### Minor Changes - [#1376](#1376) [`3ee100add`](3ee100a) Thanks [@benchristel](https://github.com/benchristel)! - Implement the protractor for Mafs interactive graphs - [#1381](#1381) [`26dceb8d7`](26dceb8) Thanks [@benchristel](https://github.com/benchristel)! - Make the `mafs.point` flag control whether point graphs with fixed numbers of points should use Mafs. Previously, turning on the `mafs.point` flag would enable Mafs for point graphs with unlimited points as well. ### Patch Changes - [#1358](#1358) [`93eeda1e2`](93eeda1) Thanks [@benchristel](https://github.com/benchristel)! - Add TODO comment - [#1379](#1379) [`685fa9048`](685fa90) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Moving around/renaming components so they make more sense for the upcoming hint mode work - [#1370](#1370) [`48e879ace`](48e879a) Thanks [@benchristel](https://github.com/benchristel)! - Internal: copy Mafs' implementation of useMovable into our own useDraggable hook. ## @khanacademy/perseus-editor@7.0.1 ### Patch Changes - [#1375](#1375) [`a8b3aa9c0`](a8b3aa9) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Fix the broken storybook preview for segments, points, and polygons - [#1379](#1379) [`685fa9048`](685fa90) Thanks [@nishasy](https://github.com/nishasy)! - [Interactive Graph Editor] Moving around/renaming components so they make more sense for the upcoming hint mode work - Updated dependencies \[[`3ee100add`](3ee100a), [`93eeda1e2`](93eeda1), [`685fa9048`](685fa90), [`48e879ace`](48e879a), [`26dceb8d7`](26dceb8)]: - @khanacademy/perseus@24.1.0 Author: khan-actions-bot Reviewers: benchristel Required Reviewers: Approved By: benchristel Checks: ✅ codecov/project, ✅ codecov/patch, ✅ Upload Coverage (ubuntu-latest, 20.x), ⏭️ Publish npm snapshot, ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Jest Coverage (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ gerald Pull Request URL: #1377

benchristel added 3 commits June 21, 2024 11:53

Copy useMovable implementation into useDraggable

96662b8

Add tests

e94bd29

Add changeset

de05f4e

benchristel self-assigned this Jun 21, 2024

benchristel requested review from mark-fitzgerald, Myranae and SonicScrewdriver June 21, 2024 19:23

khan-actions-bot requested a review from a team June 21, 2024 19:23

benchristel changed the title ~~Create our own wrapper around Mafs' useMovable~~ Copy the implementation of useMovable into useDraggable Jun 21, 2024

benchristel commented Jun 21, 2024

View reviewed changes

Myranae approved these changes Jun 21, 2024

View reviewed changes

catandthemachines approved these changes Jun 24, 2024

View reviewed changes

benchristel added 4 commits June 25, 2024 16:27

Use a button element for TestDraggable

28a0e4e

Add tests for dragging and constraints

a123a0e

Add test for userTransform

0dc02f4

Add test for keyboard movement

652c5b8

benchristel merged commit 48e879a into main Jun 26, 2024
21 checks passed

benchristel deleted the benc/copy-in-useMovable branch June 26, 2024 20:31

khan-actions-bot mentioned this pull request Jun 26, 2024

Version Packages #1377

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Copy the implementation of `useMovable` into `useDraggable` #1370

Copy the implementation of `useMovable` into `useDraggable` #1370

benchristel commented Jun 21, 2024 •

edited

Loading

khan-actions-bot commented Jun 21, 2024

github-actions bot commented Jun 21, 2024 •

edited

Loading

benchristel Jun 21, 2024

benchristel Jun 21, 2024

Myranae Jun 21, 2024

benchristel Jun 24, 2024

benchristel Jun 26, 2024

github-actions bot commented Jun 21, 2024 •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading

Myranae left a comment

Myranae Jun 21, 2024

benchristel Jun 24, 2024

Myranae Jun 21, 2024

catandthemachines left a comment

benchristel commented Jun 26, 2024

Copy the implementation of useMovable into useDraggable #1370

Copy the implementation of useMovable into useDraggable #1370

Conversation

benchristel commented Jun 21, 2024 • edited Loading

Summary:

khan-actions-bot commented Jun 21, 2024

Gerald

github-actions bot commented Jun 21, 2024 • 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

github-actions bot commented Jun 21, 2024 • edited Loading

codecov bot commented Jun 21, 2024 • edited Loading

Codecov Report

Myranae 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

catandthemachines left a comment

Choose a reason for hiding this comment

benchristel commented Jun 26, 2024

Copy the implementation of `useMovable` into `useDraggable` #1370

Copy the implementation of `useMovable` into `useDraggable` #1370

benchristel commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024 •

edited

Loading

codecov bot commented Jun 21, 2024 •

edited

Loading