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

Modified Examples to work without systemjs #9904

Merged
merged 1 commit into from
Jul 22, 2018

Conversation

RonLek
Copy link
Contributor

@RonLek RonLek commented Jul 20, 2018

Closes #9784

@timvandermeij @Snuffleupagus Please review for changes, if any. Thanks!

README.md Outdated
@@ -104,7 +104,7 @@ You can play with the PDF.js API directly from your browser using the live demos

The repository contains a hello world example that you can run locally:

+ [examples/helloworld/](https://github.com/mozilla/pdf.js/blob/master/examples/helloworld/)
+ [examples/learning/helloworld](https://github.com/mozilla/pdf.js/blob/master/examples/learning/helloworld.html)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that this example cannot be run without building PDF.js first, the "The repository contains a hello world example that you can run locally:" is no longer entirely accurate (and may just be confusing to users). Furthermore, the general examples folder is already mentioned just below.

Hence I'd suggest simply removing lines https://github.com/mozilla/pdf.js/blame/master/README.md#L105-L107 instead; @timvandermeij would you agree?


Also, somewhat unrelated to the above, line https://github.com/mozilla/pdf.js/blame/master/docs/contents/examples/index.md#L8 needs to be updated to point to https://github.com/mozilla/pdf.js/blob/master/examples/learning/helloworld.html instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I fully agree with this.

<script src="../../node_modules/systemjs/dist/system.js"></script>
<script src="../../systemjs.config.js"></script>
<script src="../../node_modules/pdfjs-dist/build/pdf.js"></script>
<script src="../../node_modules/pdfjs-dist/web/pdf_viewer.js"></script>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole point of updating the SVG example is to have it utilize the viewer components instead.

Hence this file needs to be replaced with a (almost) verbatim copy of https://github.com/mozilla/pdf.js/blob/master/examples/components/simpleviewer.html instead. The one, and only, difference is that the <title> tag should read <title>PDF.js SVG viewer using built components</title>.

alert('Please build the pdfjs-dist library using\n' +
' `gulp dist-install`');
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole point of updating the SVG example is to have it utilize the viewer components instead.

Hence this file needs to be replaced with a (almost) verbatim copy of https://github.com/mozilla/pdf.js/blob/master/examples/components/simpleviewer.js instead. The one, and only, difference is that lines https://github.com/mozilla/pdf.js/blob/master/examples/components/simpleviewer.js#L41-L44 needs to be changed to the following instead:

var pdfViewer = new pdfjsViewer.PDFViewer({
  container: container,
  linkService: pdfLinkService,
  renderer: 'svg',
  textLayerMode: 0,
});

@RonLek
Copy link
Contributor Author

RonLek commented Jul 21, 2018

@Snuffleupagus Thanks for reviewing. Made the changes!

README.md Outdated
The repository contains a hello world example that you can run locally:

+ [examples/helloworld/](https://github.com/mozilla/pdf.js/blob/master/examples/helloworld/)

More examples can be found in the examples folder. Some of them are using the pdfjs-dist package, which can be built and installed in this repo directory via `gulp dist-install` command.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe it wouldn't be a bad idea to change the "examples folder" text into a link, i.e. like so:

[examples folder](https://github.com/mozilla/pdf.js/tree/master/examples/)

@RonLek
Copy link
Contributor Author

RonLek commented Jul 21, 2018

And Done!!

@timvandermeij timvandermeij merged commit 1aaeaf3 into mozilla:master Jul 22, 2018
@timvandermeij
Copy link
Contributor

Nice work; thank you for your contribution!

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.

3 participants