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 horizontal scrolling (hold shift) #2432

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Support horizontal scrolling (hold shift) #2432

wants to merge 2 commits into from

Conversation

towerofnix
Copy link
Contributor

@towerofnix towerofnix commented Jun 29, 2019

This PR makes Snap! scroll elements horizontally when you hold shift - it's a standard interaction in almost all desktop and web apps. It also make it so Snap! uses the standardized 'wheel' event, instead of non-standard/obsolete alternatives.

In actuality, this PR is extremely similar to one I made for Scratch 3.0: scratchfoundation/scratch-blocks#1670.

Tested in Firefox and Chromium! Definitely worth checking on a device with a trackpad (i.e. a macbook) before merging; I haven't got access to one ATM, so I haven't tried myself.

Relevant forum thread: https://forum.snap.berkeley.edu/t/holding-shift-while-scrolling-should-scroll-horizontally/604/3

/cc @cycomachead!

Also makes use of the standardized 'wheel' event, instead of
non-standard/obsolete alternatives.
@cycomachead
Copy link
Collaborator

Well I certainly like that this adds features while reducing code. :)

Minor thing but do you have a link to the reasons for the multiplier values?

@towerofnix
Copy link
Contributor Author

Minor thing but do you have a link to the reasons for the multiplier values?

There's no hard resource explaining it, but there's some discussion in scratchfoundation/scratch-blocks#1190. In actuality we were already basically doing this math in the existing Snap! code - note we were dividing by 120 or 3 depending on if the event was from Chrome or Firefox. The point is just to adjust the value to a range that Snap! is roughly expecting, instead of some really giant value like 50, which would result in a very big scroll distance. :P

@jmoenig
Copy link
Owner

jmoenig commented Jul 1, 2019

Thank you!

@towerofnix
Copy link
Contributor Author

No problem! 🎉

@cycomachead cycomachead self-assigned this Jul 8, 2019
src/morphic.js Outdated Show resolved Hide resolved
@cycomachead
Copy link
Collaborator

Sorry for the delay, tested and test seems fine on a Mac with a trackpad.

@towerofnix
Copy link
Contributor Author

No worries! Thanks for the review. I made the fix you suggested (plus a semicolon missing from your suggested changes :P).

@cycomachead
Copy link
Collaborator

Good catch. 👀

@cycomachead cycomachead self-requested a review July 30, 2019 04:14
Copy link
Collaborator

@cycomachead cycomachead left a comment

Choose a reason for hiding this comment

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

This seems good to me. :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants