-
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
Copy the implementation of useMovable
into useDraggable
#1370
Changes from 3 commits
96662b8
e94bd29
de05f4e
28a0e4e
a123a0e
0dc02f4
652c5b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"@khanacademy/perseus": patch | ||
--- | ||
|
||
Internal: copy Mafs' implementation of useMovable into our own useDraggable hook. |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These tests are unfortunately incomplete. I couldn't figure out how to get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I tried what those docs suggest. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: I looked at the tests for |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
import {render, screen} from "@testing-library/react"; | ||
import {userEvent as userEventLib} from "@testing-library/user-event"; | ||
import {Mafs} from "mafs"; | ||
import * as React from "react"; | ||
import {useRef} from "react"; | ||
|
||
import {useDraggable} from "./use-draggable"; | ||
|
||
import type {vec} from "mafs"; | ||
|
||
function TestDraggable(props: { | ||
point: vec.Vector2; | ||
constrain: (point: vec.Vector2) => vec.Vector2; | ||
}) { | ||
const gestureTarget = useRef<HTMLParagraphElement>(null); | ||
const {dragging} = useDraggable({ | ||
...props, | ||
gestureTarget, | ||
onMove: () => {}, | ||
}); | ||
return ( | ||
<p role="button" ref={gestureTarget}> | ||
dragging: {String(dragging)} | ||
</p> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: Is it okay to set the role of a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's valid, but potentially confusing. I should probably just make this a button element. |
||
); | ||
} | ||
|
||
describe("useDraggable", () => { | ||
let userEvent; | ||
beforeEach(() => { | ||
userEvent = userEventLib.setup({ | ||
advanceTimers: jest.advanceTimersByTime, | ||
}); | ||
}); | ||
|
||
it("initially returns {dragging: false}", () => { | ||
render( | ||
<Mafs width={200} height={200}> | ||
<TestDraggable point={[0, 0]} constrain={(p) => p} /> | ||
</Mafs>, | ||
); | ||
|
||
expect(screen.getByText("dragging: false")).toBeInTheDocument(); | ||
}); | ||
|
||
it("returns {dragging: true} when the mouse button is held down", async () => { | ||
render( | ||
<Mafs width={200} height={200}> | ||
<TestDraggable point={[0, 0]} constrain={(p) => p} /> | ||
</Mafs>, | ||
); | ||
const dragHandle = screen.getByRole("button"); | ||
|
||
// Act | ||
await userEvent.pointer({keys: "[MouseLeft>]", target: dragHandle}); | ||
|
||
// Assert | ||
expect(screen.getByText("dragging: true")).toBeInTheDocument(); | ||
}); | ||
|
||
it("returns {dragging: false} when the mouse button is released", async () => { | ||
render( | ||
<Mafs width={200} height={200}> | ||
<TestDraggable point={[0, 0]} constrain={(p) => p} /> | ||
</Mafs>, | ||
); | ||
const dragHandle = screen.getByRole("button"); | ||
|
||
// Act | ||
await userEvent.pointer([ | ||
{keys: "[MouseLeft>]", target: dragHandle}, | ||
{keys: "[/MouseLeft]"}, | ||
]); | ||
|
||
// Assert | ||
expect(screen.getByText("dragging: false")).toBeInTheDocument(); | ||
}); | ||
}); |
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.
@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.