-
Notifications
You must be signed in to change notification settings - Fork 105
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
New: Enhanced pinch to zoom #567
Conversation
Verified that @jeremypress has signed the CLA. Thanks for the pull request! |
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.
Looks great, other than I think there's some duplicated code between some start/change/end functions.
src/lib/viewers/doc/DocBaseViewer.js
Outdated
* @return {void} | ||
*/ | ||
pinchToZoomEndHandler() { | ||
if (this.zoomWrapper && this.isPinching) { |
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.
To remove a level of arrowness, you could do if(!this.zoomWrapper || !this.isPinching) return
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.
Looks great, after comments! Good job :)
src/lib/util.js
Outdated
} | ||
} | ||
|
||
return closestPage[0]; |
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.
Careful, this'll error out if closestPage
is never set
src/lib/viewers/doc/DocBaseViewer.js
Outdated
this.xOffset = xOrigin; | ||
this.y = touchMidpoint[1]; | ||
this.x = touchMidpoint[0]; | ||
// |
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.
empty comment
src/lib/viewers/doc/DocBaseViewer.js
Outdated
// PDF.js zoom | ||
this.pdfViewer.currentScaleValue = this.pdfViewer.currentScale * this.pinchScale; | ||
|
||
// // Scroll to correct position after zoom |
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.
extra //
in comment (unless github is acting up)
as well as commented code
Still a WIP, but this PR adds a more fluid pinch-to-zoom by:
Open Issues: