LycheeOrg / Lychee-front

JS implementation of Lychee frontend
https://lycheeorg.github.io/
MIT License
48 stars 53 forks source link

Fix toggle of header #188

Closed tmp-hallenser closed 4 years ago

tmp-hallenser commented 4 years ago

Fix toggling of header in 2 cases

  1. single touch does not toggle header (#174) Event 'mousemove' gets called after a single touch and 'mousemove' makes header appear; Solution is to prevent event 'mousemove' to be called after 'touchend'

  2. swipe forward / backward toogles header Swiping forward / backward triggers 'touchend' event, which toggles the header. Solution is to prevent toggling if 'swipeend' triggered loading of previous/next photo

sonarcloud[bot] commented 4 years ago

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

ildyria commented 4 years ago

:astonished: Congrat for finding that one.

tmp-hallenser commented 4 years ago

Btw, I am across a couple of code snipped where I didn't see the logic:

  1. What's the purpose of this line? https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/main/init.js#L102 The events in the next section will not be triggered if you don't have a touch device.

  2. Duplicates There seem to be a duplicates: https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/view/main.js#L191 https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/main/init.js#L28 and https://github.com/LycheeOrg/Lychee-front/blob/7b21ec0b5fc573ddf72f02f7f80a5a9aee51ea80/scripts/main/lychee.js#L659 https://github.com/LycheeOrg/Lychee-front/blob/7b21ec0b5fc573ddf72f02f7f80a5a9aee51ea80/scripts/frame/main.js#L60

  3. Hybrid devices We bind many event to either click or touchend by this function: https://github.com/LycheeOrg/Lychee-front/blob/7b21ec0b5fc573ddf72f02f7f80a5a9aee51ea80/scripts/frame/main.js#L60 This falls short in case of convertible laptops, e.g. the X1 Carbon Yoga series. Why do we need this function at all? Shouldn't click be also triggered on a touch device (although a bit later in the event cascade). Furthermore, as a convertible user, I would expect the app to behave the same if I use the mouse of my finder to navigate (in particular these lines https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/main/init.js#L104-L117)

ildyria commented 4 years ago

Tested. Did not see any problem.

ildyria commented 4 years ago

Btw, I am across a couple of code snipped where I didn't see the logic:

  1. What's the purpose of this line? https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/main/init.js#L102

    The events in the next section will not be triggered if you don't have a touch device.

No clue.

  1. Duplicates There seem to be a duplicates: https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/view/main.js#L191

    https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/main/init.js#L28

    and https://github.com/LycheeOrg/Lychee-front/blob/7b21ec0b5fc573ddf72f02f7f80a5a9aee51ea80/scripts/main/lychee.js#L659

    https://github.com/LycheeOrg/Lychee-front/blob/7b21ec0b5fc573ddf72f02f7f80a5a9aee51ea80/scripts/frame/main.js#L60

Gotta remove them then.

  1. Hybrid devices We bind many event to either click or touchend by this function: https://github.com/LycheeOrg/Lychee-front/blob/7b21ec0b5fc573ddf72f02f7f80a5a9aee51ea80/scripts/frame/main.js#L60

    This falls short in case of convertible laptops, e.g. the X1 Carbon Yoga series. Why do we need this function at all? Shouldn't click be also triggered on a touch device (although a bit later in the event cascade). Furthermore, as a convertible user, I would expect the app to behave the same if I use the mouse of my finder to navigate (in particular these lines https://github.com/LycheeOrg/Lychee-front/blob/fe0cb5415748340fc6529f5c623bf289c1cdd33f/scripts/main/init.js#L104-L117 )

Feel free to refactor that, I don't have a strong opinion. :smiley: