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

Brush interaction not working on mobile #845

Open
BrianRosamilia opened this issue Oct 6, 2020 · 17 comments · May be fixed by #1874
Open

Brush interaction not working on mobile #845

BrianRosamilia opened this issue Oct 6, 2020 · 17 comments · May be fixed by #1874

Comments

@BrianRosamilia
Copy link

BrianRosamilia commented Oct 6, 2020

(Enjoying the library, thanks for making it available)

I noticed the brush doesn't work with touch interactivity on mobile. Example here: https://vx-demo.now.sh/brush

After digging into the code a bit, I noticed that mobile events don't seem to be wired up for the Drag component. Shouldn't drag include touch and touchmove the same way as mouse and mousemove?

https://github.com/airbnb/visx/blob/master/packages/visx-drag/src/Drag.tsx#L121-L122

I could give it a shot if this is unintended and no one has time to look into it or is currently doing so. Lmk

@williaster
Copy link
Collaborator

Hey @BrianRosamilia thanks for the bug report 🙏 I think this is simply an oversight with @visx/drag, it should support the touch events as you noted.

Would happily review a PR if you want to take a shot!

@BrianRosamilia
Copy link
Author

BrianRosamilia commented Oct 9, 2020

@williaster

Can you (or someone) help me build the brush and its dependencies?

Interestingly, the build for the brush points at the lib folder of @visx/drag and not at drag directly, which I'm noticing almost no other components do--so the instructions for building/debugging a single component don't actually work, as I see it :( I can work on the brush by following the instructions, but when I try to modify drag it doesn't work.

https://github.com/airbnb/visx/blob/master/packages/visx-brush/src/BaseBrush.tsx#L4

Feel free to just show me a bash script to build brush & its dependencies properly, I'm guessing this has been solved before!

Edit: you can just use yarn build to avoid this but it builds all of the components

@m9dfukc
Copy link

m9dfukc commented Oct 20, 2020

@BrianRosamilia did you make any progress with this? also came here because I noticed that the brush is broken on mobile :/

edit: Sorry totally missed your pull request 🤦 ... anyway, is there a chance that this gets a review (and merge) soon?

@iampueroo
Copy link
Contributor

@williaster (quick FYI as I was perusing the open bugs again) - I believe this issue can be immediately closed, since #1155 was merged.

@shripadk
Copy link

Has this issue actually been fixed? I tried it on my mobile now (Android) and it isn't working.

@williaster
Copy link
Collaborator

I just played with it on mobile/android also, the brush moves but it's not a smooth experience/you can't really get it to do what you want. so looks like there's maybe some more validation/improvements that need to be made for that case.

@williaster
Copy link
Collaborator

williaster commented Jun 30, 2021

Yep found a couple of refs that still have onMouse... in @visx/brush, should be a simple fix if anyone is interested!

e.g.,

onMouseDown={dragStart}

@williaster
Copy link
Collaborator

closed by #1286

@sakulstra
Copy link
Contributor

@williaster are you sure this is fixed?
I tried on https://airbnb.io/visx/brush with my mobile device and it still seems to behave somewhat random.

@williaster
Copy link
Collaborator

yeppp, still not working I agree 🤦

@williaster williaster reopened this Jul 27, 2021
@Stuhl
Copy link

Stuhl commented Nov 23, 2021

Any progress?

@RyKilleen
Copy link
Contributor

Would love to renew this issue.

It was an assumption of mine that this would work on mobile. Happy to put in some work if anyone knows the clear fix, or to spend some time identifying it.

@rubo
Copy link

rubo commented Mar 28, 2024

Given that everything is mobile-first nowadays, it's a pity this issue hasn't been addressed yet. Could this be prioritized, please?

@hugolgst
Copy link

updates?

@eliza-guseva
Copy link

eliza-guseva commented Aug 21, 2024

Upd. I just have made a build with the original Visx and the brush is working on mobile. So my PR is probably unnecessary.
I am not sure why it wasn't working before and why it is working now. But maybe if you have time, you could look to see if the PR still makes sense.


There were a couple more explicit onMouse... mentions in some Brush files. I believe the PR should fix the issue. At least with my forked version my brushes in my graphs work on mobile

@hugolgst
Copy link

not sure what you did to make it work on mobile, with the latest version, for me, it still does not work

@kachkaev
Copy link
Contributor

👋 @williaster WDYT of #1874? It’d be great to see something like that released to improve brush UX on touch devices. Not sure if there is an alternative until this issue is fixed, unless implementing Brush from scratch locally. That’d be quite laborious and error prone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet