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

Loading indicator #15

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

jazzido
Copy link
Contributor

@jazzido jazzido commented Aug 14, 2014

This is a proposed implementation of one of the features on the wishlist. Some notes:

  • The spinner is implemented as an animated gif, which I took from the PDF.js example viewer
  • It's embedded in the JS source as a base64-encoded data url.
  • To center the spinner on each page DOM element, I set the position style attribute of the .plv-page-container.page-container divs as relative. That might cause problems when integrating pdfListView on an already styled page. A possible alternative is to set position: absolute on the loading indicator and calculate its position with PageView.getCanvasPositionInViewer.

Feel free to merge :)

@jviereck
Copy link
Owner

@jazzido, thanks for proposing this PR. This is definitely a feature worth adding. However, I don't think the PR is good to be merged the way it is at the moment.

My main concern is, that I want the pdfListViewer to be "flexible, modular, easy to replace parts with other implementations". This is not really given with your implementation, as the spinner is hard coded ;)

The way the textLayer and annotationLayer are integrated is already flexible to some extend. Maybe it's worth adding a bit of infrastructure to add support for spinners as well. Here is what I think:

  1. add a event library to pdfListView, such that object (e.g. PageView can emit and listen to event)
  • choose a library that allows to bubble events up to an object's parent (e.g. an event dispatched on the PageView should dispatch it on the listView it is owned by)
    2) emit event like render-setp-context, render-begin, render-done etc. on the pageView
    3) make the textLayer, annotationLayer listen to these events
    4) implement a new "spinner" object also by listening to these events

@jazzido, do you see where I am heading? Let me know what you think. If you want I can do steps 1)-3) from above and then leave the stage for you to do 4)?

@jazzido
Copy link
Contributor Author

jazzido commented Aug 20, 2014

@jazzido, do you see where I am heading? Let me know what you think. If you want I can do steps 1)-3) from above and then leave the stage for you to do 4)?

That sounds about right. Adding an event library to pdfListView would require substantial changes so I'd better leave that to you. I'll be happy to hook a spinner when that's done.

Thanks!

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

Successfully merging this pull request may close these issues.

2 participants