-
Notifications
You must be signed in to change notification settings - Fork 9
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
Touch handling for sky browser #134
base: master
Are you sure you want to change the base?
Conversation
# Conflicts: # src/components/BottomBar/SkyBrowser/WorldWideTelescope.jsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It almost works, but it both moves and zooms when pinching. The zooming is also "jumpy", but I think that might be partly/mainly because of the both the move and pinching is triggered simultaneously. See video
Note that when using multiple fingers, the startInteraction
function is called when the first finger touches the screen. I think this might the reason for the behavior, since for the first finger, it will register as dragging... But somehow, some zooming still happens 🤔
I would offer to look at it but can't prioritize this right now, unfortunately
Project.Name.mp4
return position; | ||
} | ||
|
||
function scrollZoom(scroll) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "scroll" value here is a bit ambiguous. What doe sit mean? Does it deserve a more describing name?
else { // mouse | ||
position = [interaction.clientX, interaction.clientY]; | ||
} | ||
if (!position) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if-statement seems unnecessary. If !position
is true, it means that it's undefined, which means that we will return undefined even if we just return position as is
skybrowserApi.stopAnimations(browserId); | ||
} | ||
|
||
function handleDrag(interaction) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this is nitpicky, but as this is an event handler function, I would prefer to call the interaction
variable event
instead. To make it clear that it is actually an object of the javascript event type :) The same goes for all the interaction
variables in this file
} | ||
|
||
function handleDrag(interaction) { | ||
const end = getClientXY(interaction); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a position, or rather, multiple positions, the name should reflect that. It's also the current position rather than the end position
Just positions
?
Can confirm. Moving with touch works great, but when pinching, it looks like the movement of the first (?) finger is not disregarded when doing the pinch-to-zoom |
This PR features touch handling for skybrowser
Pinching needs to be tested on a touchscreen