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

Implement useDrag hook #1478

Merged
merged 1 commit into from
Aug 31, 2023
Merged

Implement useDrag hook #1478

merged 1 commit into from
Aug 31, 2023

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented Aug 18, 2023

This should hopefully make implementing draggable interactions easier (e.g. moving/resizing ROIs), though I'm hoping it will also abstract out a lot of the code from SelectionTool and Pan.

The approach taken by @visx/drag is not great for us because of our multiple coordinate spaces and because it requires capturing events on a transparent rectangle on top of the element that is being dragged.

Instead, the approach I'm proposing makes use of the new event bubbling mechanism introduced in #1473: useDrag automatically registers the pointermove and pointerup event handlers on canvasWrapper with useCanvasEvents, thus making sure that no events are missed. The consumer only needs to register a pointerdown handler on the draggable element.

I've created a story to demonstrate the use of useDrag and how pretty straightforward it is:

Peek 2023-08-18 16-29

@axelboc axelboc marked this pull request as draft August 18, 2023 14:48
Base automatically changed from event-bubbling to main August 21, 2023 13:55
@axelboc axelboc force-pushed the usedrag branch 2 times, most recently from 41492f3 to 469ffda Compare August 25, 2023 09:48
@axelboc axelboc marked this pull request as ready for review August 25, 2023 09:48
@axelboc axelboc requested a review from loichuder August 25, 2023 09:49
@axelboc axelboc force-pushed the usedrag branch 4 times, most recently from 0a30281 to 4f117f6 Compare August 30, 2023 13:00
@axelboc
Copy link
Contributor Author

axelboc commented Aug 30, 2023

@loichuder I've moved the useDrag story under Experimental as the hook is likely to evolve a lot over the next few releases.

Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

Looks good !

I wonder if we should not expose a ref handler that consumers will pass to the draggable element instead of having to set themselves startDrag to onPointerDown. It is just an idea: it is fine like this and more easily extendable.

apps/storybook/src/Utilities.mdx Outdated Show resolved Hide resolved
@axelboc
Copy link
Contributor Author

axelboc commented Aug 31, 2023

I wonder if we should not expose a ref handler that consumers will pass to the draggable element instead of having to set themselves startDrag to onPointerDown. It is just an idea: it is fine like this and more easily extendable.

Yeah I did consider this solution, but the problem is that we don't know enough to register the event handler ourselves: perhaps the consumer wants to check for a modifier key or a mouse button, perhaps they're implementing a canvas interaction and need to call shouldInteract, ...

@axelboc axelboc merged commit 59234c0 into main Aug 31, 2023
8 checks passed
@axelboc axelboc deleted the usedrag branch August 31, 2023 07:20
@PeterC-DLS
Copy link
Contributor

This looked handy to me until I tried to integrate it into DvdDragHandle.tsx and came across a limitation that it's tied to the r3f layer. In Davidia, each selection shape has a number of drag handles and the handles are created as children within those shapes (see, e.g., DvdAxisBox.tsx). Each shape is created as an SVGElement (see createShape).

I guess significant refactoring of the selection classes in Davidia's code may be needed to lift the drag handles creation to that createShape function which breaks a lot of my enscapsulation.

@axelboc
Copy link
Contributor Author

axelboc commented Sep 6, 2023

Thanks for sharing the link to your repo. It looks like Davidia is really taking shape (pun intended 🥁)! It's good to see another project using @h5web/lib in depth. Looking forward to seeing in action at some point!

Regarding useDrag, instead of moving your handles up, could you move SvgElement down into the shape components (e.g. into DvdAxisBox)? That being said, it is worth reconsidering whether useDrag should or shouldn't be tied with R3F.

@PeterC-DLS
Copy link
Contributor

My comment was mainly feedback but pushing down SvgElement is a good suggestion. I will see how far I get, thanks!

@PeterC-DLS
Copy link
Contributor

I think it's the fact that I create drag handles programmatically so they are not children of VisCanvas when useThree is called by useDrag (or by useCanvasEvent if I use a customized version of useDrag). The error is "R3F hooks can only be used within the Canvas component!".

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.

3 participants