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

support for touch events on mobile #23

Merged
merged 2 commits into from
Dec 6, 2022
Merged

Conversation

arminfro
Copy link
Contributor

@arminfro arminfro commented Dec 5, 2022

I've made some changes to support touch events on mobile.

If you like the changes I'd be happy to see them merged.

This would close #14

@mlejva
Copy link
Contributor

mlejva commented Dec 5, 2022

Hey, thank you for the PR! This looks good from the first glance. I'll try it out today.

I'm curious though can you elaborate on what's the use case behind the condition parameter here?
Thanks!

@arminfro
Copy link
Contributor Author

arminfro commented Dec 5, 2022

Hey, thanks for looking into it!

The condition parameter is used to not subscribe on events if it is not needed. Specifically if there is no touch support (determined by isTochDevice) then we actually don't want to add the event listener.

@mlejva
Copy link
Contributor

mlejva commented Dec 5, 2022

Hey, I just tried the your touch supported version in the example project and I'm getting some weird behavior.

Maybe I'm doing something wrong? If so, can you provide a CodeSandbox or similar example?

Here's the video of me trying to use the splitter on my phone

Thank you!

@@ -297,24 +303,33 @@ function Split({
adjustSize(direction, offset);
}, [state.isDragging, state.draggingIdx, state.pairs, adjustSize]);

function handleGutterMouseDown(gutterIdx: number, e: MouseEvent) {
e.preventDefault();
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 got an error message while dragging with touch caused by this line:
Unable to preventDefault inside passive event listener invocation

I haven't found a use case for this. Everything works without this call, is there any specific reason to call preventDefault?
If yes, I'm happy to revert and migrate this line :)

@arminfro
Copy link
Contributor Author

arminfro commented Dec 6, 2022

Thanks for trying out. I should have tested the example project with my changes.

The issue was the default scroll behaviour of the touchmove event. This is now handled with preventDefault in case of a touch event

Now I've tested the example project with firefox and chrome on mobile and on desktop.

@mlejva mlejva merged commit 83486b3 into devbookhq:master Dec 6, 2022
@mlejva
Copy link
Contributor

mlejva commented Dec 6, 2022

Just tested it and all looks good. Thank you for the PR! I'll make a new release now.

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

Successfully merging this pull request may close these issues.

Doesn't work on mobile
2 participants