-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
Added component example for single page viewer #8990
Conversation
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.
Unfortunately this doesn't (currently) work, please see the comments below.
Please remember to test locally, to make sure that the new example works, before submitting a new version of the patch (and please squash the commits).
<link rel="stylesheet" href="../../node_modules/pdfjs-dist/web/pdf_viewer.css"> | ||
|
||
<script src="../../node_modules/pdfjs-dist/build/pdf.js"></script> | ||
<script src="../../node_modules/pdfjs-dist/web/pdf_single_page_viewer.js"></script> |
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 file doesn't exist, which explains why the example currently doesn't work.
Please look at the contents of the folder in question, and you'll see that the path must be ../../node_modules/pdfjs-dist/web/pdf_viewer.js
.
Edit: See also the two existing components
examples.
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.
Also, should the condition check for PDFJS.PDFViewer
instead of PDFJS.PDFSinglePageViewer
here: https://github.com/pixelexel/pdf.js/blob/b24cdc08e24fc1e99c6a56eab807b80c15a72965/examples/components/singlepageviewer.js#L18
like in the other two examples?
Edit:
The example seems to work as intended now. Are there any tests in particular you would want me to run before I submit the patch?
|
||
// (Optionally) enable find controller. | ||
var pdfFindController = new PDFJS.PDFFindController({ | ||
pdfSinglePageViewer: pdfSinglePageViewer |
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.
I don't think this would work since, as can be seen in https://github.com/mozilla/pdf.js/blob/master/web/pdf_find_controller.js#L48, the parameter is named pdfViewer
. Hence this line needs to read pdfViewer: pdfSinglePageViewer,
.
pdfSinglePageViewer.setFindController(pdfFindController); | ||
|
||
container.addEventListener('pagesinit', function () { | ||
// We can use pdfSinglePageViewer now, e.g. let's change default scale. |
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.
Nit: Please remove two spaces of indentation here, such that the comment lines up correctly with the line below it.
package.json
Outdated
@@ -43,5 +43,8 @@ | |||
"type": "git", | |||
"url": "git://github.com/mozilla/pdf.js.git" | |||
}, | |||
"license": "Apache-2.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.
I don't understand what's happening here.
I didn't change the package.json file and still it shows as edited and the build on travis-ci seems to have failed.
Edit:
Something seemed to have gone very wrong while I was squashing the commits so I reset my branch to upstream which caused this pull request to close.
I have pushed the changes and reopened this pull request.
Sorry for all the confusion.
|
||
'use strict'; | ||
|
||
if (!PDFJS.PDFViewer || !PDFJS.getDocument) { |
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.
Why are you checking for PDFViewer
when you're not using that below?
This line should read if (!PDFJS.PDFSinglePageViewer || !PDFJS.getDocument) {
instead.
Checking for PDFJS.PDFSinglePageViewer instead Added component example for single page viewer
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 good, thank you for the patch!
Honestly, thank you for putting up with me. Looking back, I made so many stupid and obvious mistakes while working on this but you always helped me along. |
Thank you for your contribution! No worries about that; reviews are there to improve everyone's work and especially the first contribution to a project is always aimed at getting a feeling of the entire workflow, which always takes some time to get used to. You're always welcome to contribute and ask questions! |
Added component example for single page viewer
This pull request is in response to issue #8951 and adds a component example for single page viewer in the
example/components
folder.Fixes #8951.