VictoryBrushContainer: add a threshold for mouseMove events to avoid accidental minimal draws #1443
Conversation
…accidental minimal draws
| if ( | ||
| (!allowResize && !allowDrag) || | ||
| !this.withinBounds({ x, y }, fullDomainBox) || | ||
| !this.hasMoved({ ...targetProps, x2: x, y2: y }) |
There was a problem hiding this comment.
There is currently a check for whether the mouse moved in onMouseUp. I think the check should stay there, right?
There was a problem hiding this comment.
I tried to do it there initialy, but the drawing happens even before the mouseup
There was a problem hiding this comment.
Oh, I see what you mean. Since this has to run on every mouse move, it would be great to be able to short circuit it for users who don't have a mouseMoveThreshold defined. Like (targetProps.mouseMoveThreshold && !this.hasMoved({ ...targetProps, x2: x, y2: y }))
It might also be nice to reuse this logic in onMouseUp instead of the more naive check
There was a problem hiding this comment.
reuse this logic in
onMouseUp
Why? x2 won't be different from x1 unless onMouseMove handling actually happens
There was a problem hiding this comment.
General preference for a named function like hasMoved, but I guess (x1 === x2 || y1 === y2) is already pretty concise and clear.
| @@ -45,7 +45,8 @@ export const brushContainerMixin = (base) => | |||
| stroke: "transparent", | |||
| fill: "transparent" | |||
| }, | |||
| handleWidth: 8 | |||
| handleWidth: 8, | |||
| mouseMoveThreshold: 2 | |||
There was a problem hiding this comment.
The default value for mouseMoveThreshold should be 0
|
@Hypnosphi Thanks! I'll get a patch out for you tonight :) |
When using
defaultBrushArea: 'move', it's too easy to accidentally draw a minimal selection when in fact you just click the empty area. I've added a customisable threshold for handling mouseMove with default of 2pxProbably related: #1058