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

feat(mobile): add touch delay to allow scroll on mobile #315

Merged
merged 11 commits into from
Sep 4, 2019

Conversation

begner
Copy link
Contributor

@begner begner commented Aug 27, 2019

Description

This is a fix for touch capable devices, there drift zoom prevents scrolling while displayed on screen. The Drift Stage cant be use for start oft gestures, because the event will always be
default-prevented. Its not breaking because you need to set touchDelay (> 0) for this to work. IF there is touchdelay set, drift only prevents browsers default behaviour if the zoom is active. otherwise the events will pass. Fixes #77

Bug Fix

  • All existing unit tests are still passing (if applicable)
  • Add some steps so we can test your bug fix
  • Update the readme

Steps to Test:

  • Create an (large) image which has drift zoom applied on.
  • Configure drift with no touchDelay.
  • Add some "lorem ipsum" text below.
  • Use a touch capable mobile (iphone X here)
  • open the page on a mobile an make sure the image covers the complete display
  • try to scroll down
    You cant!

With fix:

  • set touchDelay to 500

  • try to scroll down (while gesture starts on the image)
    You can scroll now!

  • holding you finger on the image on the image for > 500ms
    now you can move around the zoom maginfication bubble without scrolling

…mobile, while image with drift zoom covers the screen.
@begner
Copy link
Contributor Author

begner commented Aug 27, 2019

Hm - the tests wont run on windows - so i cant test it locally. Anyway it ist strange - because the touchDelay parameter is passed and should be present.

Uncaught TypeError: Cannot read property 'touchDelay' of undefined thrown

begner and others added 6 commits August 28, 2019 10:54
…mobile, while image with drift zoom covers the screen.
…mobile, while image with drift zoom covers the screen.
…mobile, while image with drift zoom covers the screen.
…mobile, while image with drift zoom covers the screen.
@begner begner changed the title Fix for touch on mobile, which does not allows the user to scroll on … fix(scroll on mobile): fixed bug #77 Aug 28, 2019
@sherwinski
Copy link
Contributor

Hey, thanks for the PR @begner!
I see you have test steps included, do you happen to have something like a public codepen demo I can use to try this out myself? I'll try to have this PR reviewed for you by the end of the week.

@begner
Copy link
Contributor Author

begner commented Aug 29, 2019

do you happen to have something like a public codepen demo I can use to try this out myself?

Sure

Visit with your mobile and try to scroll down.

Thats the issue:
https://codepen.io/begner/full/vYBZJJB
It instantly does inpane-zooming. No scrolling possible

Thats with the fix:
https://codepen.io/begner/full/PoYjKyb
You can scroll. If you touch and dont move your finger for a small amount of time, the zoom will kick in (while its visible. you wont scroll when moving your finger around)

Note: For IOS i suggest to add:

      imageElement.addEventListener('touchforcechange', function (event) {
        if (event.changedTouches[0].force > 0.1) {
          event.preventDefault()
          return false
        }
      })

to prevent ios-specific pop up images on "hard" press (force-touch, 3d touch - or whatever their marketing name is for that)

Copy link
Contributor

@sherwinski sherwinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome @begner! The solution works really well.
Only one small thing, can you please uncommit the dist/ files? We generally build everything at the end when we release a new version of the library. Thanks

This reverts commit c491e34
@begner
Copy link
Contributor Author

begner commented Sep 2, 2019

uncommit the dist/ files?

Done - was a mistake, should have been in a branch. :D

@sherwinski
Copy link
Contributor

@begner no problem and thanks!
Out of curiosity, do you think there's someway we can add a test case for this to make sure future work doesn't break it?

@begner
Copy link
Contributor Author

begner commented Sep 4, 2019

Out of curiosity, do you think there's someway we can add a test case for this to make sure future work doesn't break it?

As long as the used headless-test-browsers does not support REAL mobile behaviours (with real touchevents): no.

@sherwinski
Copy link
Contributor

Fair enough, besides that I think this PR is good to go. Thanks for putting in the time to address my feedback @begner

@sherwinski sherwinski changed the title fix(scroll on mobile): fixed bug #77 feat(mobile): add touch delay to allow scroll on mobile Sep 4, 2019
@sherwinski sherwinski merged commit ceb6101 into strawdynamics:master Sep 4, 2019
@begner
Copy link
Contributor Author

begner commented Sep 5, 2019

@sherwinski
Thanks for your time, too.

Can you drop me a note, when the npm version is released?

@sherwinski
Copy link
Contributor

@begner
Will do, I just need to finish fixing one other bug so I can release them together. I am aiming to have it done in the next few days.

@sherwinski
Copy link
Contributor

Great news @begner, this feature is now released as part of Drift v1.4.0 🎉

@begner
Copy link
Contributor Author

begner commented Sep 9, 2019

released as part of [Drift v1.4.0]

👍

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.

Introduce a delay to allow scroll on mobile
3 participants