feat(brush): add using widows move events with brush

[d3x42]

The new param isUseWindowMoveEvents will allow window event listeners to control dragging move and end.

🚀 Enhancements

It's possible to move the cursor away from the brush area (and finish brushing there).

Related PR and issues:
PR: PR-1110
Issue: #1109

My goal here - in the "isUseWindowMoveEvents" mode handle all mouse movements and mouse up events on the Base Brush component level. If there are examples for BrushCorner in the gallery I could try to handle it too.

[d3x42]

Oh, it seems that my linter didn't get all issues here. I'll provide lint fixes tomorrow.

[d3x42]

@williaster Ready for review. Thanks!

[williaster]

Hey @d3x42 thanks for getting this started!

I had a few specific comments to start, and my main high-level question is whether we need to introduce isMovingBrushSelection and brushHandleChange, or whether we can simply use the existing isBrushing flag. If you could explain this design choice more it'd be helpful.

[d3x42]

Hey @d3x42 thanks for getting this started!

I had a few specific comments to start, and my main high-level question is whether we need to introduce isMovingBrushSelection and brushHandleChange, or whether we can simply use the existing isBrushing flag. If you could explain this design choice more it'd be helpful.

It was separated because I need to determine which event would be handled by the mouse move event listener (logic from move handlers in the child components slightly different). I think that I could use one enum variable instead of multiple boolean. Something like brushingType: Moving|Brushing|Resize_left|Resize_Right etc.

[d3x42]

@williaster It seems that I resolved all of your comments. Please check when you have time.

[williaster]

Hey @d3x42 sorry for such a long delay, have been super slammed at work and out a bit on vacation. Will try to get this reviewed by end of week 🙏

[d3x42]

@williaster gentle ping

[williaster]

Hey @d3x42 thanks for the ping. I just pulled this branch locally to play with it functionally (by setting useWindowMoveEvents=true on the <Brush /> in the demo package) and there seem to be several issues still.

Cannot resize the brush

Jumpy-ness when moving the brush on the track
It always jumps to the right side of the brush when you first move it

Jumpy-ness when moving the brush outside the track

How were you testing this functionally? I think these will need to be fixed before we can merge it, but thanks again for all the work on improving this 🙏

[d3x42]

It's strange. Resize works for me:

[d3x42]

@williaster Sorry for the bug with jumping on move starts, it should be fine now.

Resize works fine for me. I checked it in chrome, firefox and safari on macOS. Could you add some details on your os, browser, and browser version to reproduce it?

[williaster]

Hey @d3x42 I pulled it again and it's no longer jumpy/works well when dragging outside of the svg 🎉.

Still can't resize tho, I tried it on chrome/firefox/safari and same behavior for each. I'm on macOS BigSur 11.3.1. I wanted to confirm that you were rebuilding both @visx/brush and @visx/drag locally when testing? It's a little confusing because the @visx/demo site seems to use the esm build for direct imports (brush) and non-esm builds for dependencies of those imports (drag).

So locally I was running yarn dev in the demo package, then rebuilding with two commands
yarn build:workspaces --workspaces=@visx/brush --esm
yarn build:workspaces --workspaces=@visx/drag

To verify, when yarn dev is running for @visx/demo I usually check that the next server re-compiles when running either of those.

[d3x42]

@williaster Thanks! After rebuilding @visx/drag without esm I was able to reproduce the issue. Fix already here.

[williaster]

Hey @d3x42 I can resize now, but noticed a jumping issue during the transition between the svg/window. It seems to only happen on the left side.

[d3x42]

@williaster I fixed jumpings on resizing and selection

[d3x42]

@weibohe Hi! Any news on the review for this PR?

[d3x42]

Merge master changes to branch.

@williaster could you please take a look on this pr?

[d3x42]

@williaster gentle ping

[williaster]

Hey @d3x42 sorry for the delay on my end, I have been super busy but I appreciate the pings. I think this is close, just a couple more comments on my end for readability.

[williaster]

don't worry about happo failing, we are having some auth issues with it.

[williaster]

also played with it functionally and looks good, do you want to set useWindowEvents on the @visx/demo example so this is the default behavior there? 😄

[d3x42]

also played with it functionally and looks good, do you want to set useWindowEvents on the @visx/demo example so this is the default behavior there? 😄

Sure! I'll update the demo. :)

[d3x42]

@d3x42 PR updated with your suggestions.) "isControlled" definitely way better for child components in Brush.

[williaster]

Okay I think this is probably the final review. Sorry for multiple review rounds but this one is definitely more complex than some.

Thank you for all the hard work on this, it's a significant improvement to the current UX! 👏 I think after these changes we can merge this puppy! 🚢

[d3x42]

@williaster PR updated. Multiple reviews rounds fine for me, they really make this PR better.)

[williaster]

Sweet, lgtm! 😍 Thank you again 🎉

[github-actions]

🎉 This PR is included in version v1.15.0 of the packages modified 🎉

[sakulstra]

Hey, i saw this pr superseded #864 , but i'm pretty sure it doesn't.
as far as i know mouseMove will not fire on touchMove so on touch devices the brush is still not usable.
I think you can verify by visiting: https://airbnb.io/visx/brush

[williaster]

@sakulstra that's correct, this is being tracked in #845 . Fix is out now in #1286 .