Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

fix: update swipe navigation for electron native implementation of swipe events, closes #3299 #4383

Closed
wants to merge 1 commit into from

Conversation

lucidNTR
Copy link
Contributor

Because swipe events can hardly be tested currently there has been a regression with the recent electron build.

electron now implements swipe events natively with the nice side effect of detecting 3 or 2 finger swipes depending on system settings.

as far as i understand the manual swipe detection is now completely obsolete and was removed

@cndouglas
Copy link

This seems similar to PR #4369.

@darkdh
Copy link
Member

darkdh commented Oct 4, 2016

swipe event only emitted when 3-finger swiping and we should use scroll-touch-begin, scroll-touch-end and scroll-touch-edge to make sure 2-finger swiping working properly and also respect swipe system preference
If we wanna fix #3299, we need to handleswipe event for sure but also need a granular system preferences indicated user chose 3-finger-swiping.
screen shot 2016-10-04 at 10 21 51

@darkdh darkdh closed this Oct 4, 2016
@lucidNTR
Copy link
Contributor Author

lucidNTR commented Oct 4, 2016

@darkdh i see i did not handle the swiping with two fingers correctly but the merge #4369 completely breaks my 3 finger swiping. i guess we need to check for system settings and either register swipe handlers or do the scroll touch begin and end handling. could you reopen this pr so i add the missing functionality?

@darkdh
Copy link
Member

darkdh commented Oct 4, 2016

#4369 just remove legacy code deprecated by #4083. And your PR will break current two finger swiping behavior. You don't need to remove the two finger swiping code to support three finger swiping. We should first implement the system preference detection in electron and based on the preference you can handle swipe event for three finger swiping.

@darkdh darkdh reopened this Oct 4, 2016
@bsclifton
Copy link
Member

Since this has been open for a while, I'm going to close. @lucidNTR if you would like to revise this per the comment by @darkdh, please open a new PR and we'll check it out 😄 Thanks!

@bsclifton bsclifton closed this Nov 15, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants