search

airbnb / visx

šŸÆ visx | visualization components
https://airbnb.io/visx
MIT License
19.4k stars 712 forks source link

brush: fix brush jumping around after mouseup #1836

Closed tfineberg4 closed 4 months ago

tfineberg4 commented 4 months ago

:bug: Bug Fix

How to induce? resize the brush window by dragging and releasing one handle quickly, or quickly drag the brush window and release (harder to induce this way)

Impact? Handle ends up quite far away from the mouse pointer's location. Sometimes the location of both handles will be updated, resulting in a drastic resize of the brush window.

Two relevant issue have been linked below- I believe this PR should resolve both of them!

Issue 1 Issue 2

Root cause mouseup action is supposed to disable subsequent dragging of the handle upon mousemove. This is achieved by updating the brush's state. However, when moving quickly, mousemove actions can trigger handleWindowPointerMove before the state change is complete. This leads to handle movement after mouseup. Additionally, the stale state causes the occasionally erratic placement of the handle(s).

Before

https://github.com/airbnb/visx/assets/166535835/a1c82e0e-edae-449b-88ca-1259c40eb65a

After

Can't induce the bug anymore, even with very quick use of the brush.

https://github.com/airbnb/visx/assets/166535835/53bfc639-feab-4034-8e41-ac34817b13ba

wassgha commented 4 months ago

thanks šŸ‘

hshoff commented 4 months ago

Hi @tfineberg4, thanks for the PR! The before/after videos look good. Nice work.

The fix for the root problem looks right to me, but would you mind updating this PR without bringing in the @lodash/debounce dependency? Fine to just add a local debounce function in the file itself. We try to avoid dependencies if we can to keep package size down and reduce maintenance burden.

Thank you again, excited to close these long standing issues!

tfineberg4 commented 4 months ago

Hey @hshoff !

Oops, my bad! Totally thought lodash was already a dependency here. I slipped the debounce function into utils.ts. If you don't prefer that, of course I can move it into BaseBrush.tsx.

Thanks so much for an awesome set of packages, as well as your quick response here šŸ‘

github-actions[bot] commented 4 months ago

šŸŽ‰ This PR is included in version v3.10.4 of the packages modified šŸŽ‰