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.

Sign up for GitHub

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

fix(classifier): Polygon tool drag handles #6550

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+7 −2
Open

fix(classifier): Polygon tool drag handles #6550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc5a0be
fix(classifier): Polygon tool drag handles
eatyourgreens Dec 12, 2024
9324eb2
allow onFinish events to bubble up the DOM
eatyourgreens Dec 16, 2024
9f60321
decorate draggable event handlers
eatyourgreens Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/lib-classifier/src/plugins/drawingTools/components/Polygon/Polygon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ function Polygon({ active, mark, scale, onFinish }) {
strokeWidth={strokeWidth}
strokeDasharray={strokeDasharray}
/>
)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The polygon tool code is actually broken here, and has been for a while. 😳


{active &&
points.map((point, i) => (
Expand All @@ -84,7 +83,7 @@ function Polygon({ active, mark, scale, onFinish }) {
y={point.y}
fill='currentColor'
dragMove={point.moveTo}
onPointerDown={handleClosePolygon}
onPointerUp={handleClosePolygon}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this overrode onPointerDown for draggable elements, which stopped dragging from happening at all. Now it overrides onPointerUp which means dragging is never canceled once it has started. 😳

Copy link
Contributor Author

@eatyourgreens eatyourgreens Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Fixed in 4059705, but that partly reverts #3536.

Copy link
Contributor Author

@eatyourgreens eatyourgreens Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

b3b8304 might be a better fix. It decorates existing event handlers with draggable behaviour, rather than replacing or overriding them.

Copy link
Contributor Author

@eatyourgreens eatyourgreens Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Tested in Chrome and Firefox with a few different drawing tools. Seems to work OK, and dragging is now cancelled on pointer up, while also running the mark's pointerup handler.
https://localhost:8080/?project=eatyourgreens%2F-project-testing-ground&workflow=3370

Drawing while zoomed-in still triggers #6490 in Firefox, but that's a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ellipse tool still seems to work as expected in I Fancy Cats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR description too, as the scope has changed from the polygon tool to the draggable decorator.

/>
))
}
Expand Down
6 changes: 6 additions & 0 deletions packages/lib-classifier/src/plugins/drawingTools/components/draggable/draggable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ function draggable(WrappedComponent) {
dragStart = DEFAULT_HANDLER,
dragMove = DEFAULT_HANDLER,
dragEnd = DEFAULT_HANDLER,
onPointerDown,
onPointerMove,
onPointerUp,
...rest
}, ref) {
const { canvas } = useContext(SVGContext)
Expand All @@ -62,6 +65,7 @@ function draggable(WrappedComponent) {
setDragging(true)
pointerId.current = event.pointerId
dragStart({ x, y, pointerId: event.pointerId })
onPointerDown?.(event)
wrappedComponent.current?.setPointerCapture(event.pointerId)
}

Expand All @@ -76,6 +80,7 @@ function draggable(WrappedComponent) {
dragMove({ currentTarget, x, y, pointerId: event.pointerId }, difference)
coords.current = { x, y }
}
onPointerMove?.(event)
}

function onDragEnd(event) {
Expand All @@ -87,6 +92,7 @@ function draggable(WrappedComponent) {
coords.current = { x: null, y: null }
setDragging(false)
pointerId.current = -1
onPointerUp?.(event)
}

return (
Expand Down
Loading