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

A more self contained directive #18

Closed
wants to merge 1 commit into from

Conversation

winkerVSbecks
Copy link

We were using this library for a project but, had a lot of issues trying to display multiple PDFs in one view. Therefore, we decided to make a few changes to make the directive completely isolated. I'm not sure if this is something you agree with but, it might be of interest to others.

  • Made the directive a more self contained unit without polluting the parent scope.
  • Removed the need for a template by using a simple inline template.
  • The canvas element is grabbed using jqLite instead of an ID passed in.
  • The directive now has an isolated scope.
  • Switched to using a delegate service to control the directive.
  • The delegate service allows you to access and control individual instances of a directive. This allows us to have multiple instances of the same directive in the same controller.
  • All the pdf controls are available through the delegate.
  • This allows the pdf controls to be placed in any view/sub-view.

- Made the directive a more self contained unit without polluting the parent scope. Removed the need for a template and the canvas element is grabbed using jqLite instead of an ID passed in.

- The directive now has an isolated scope.

- Switched to using a delegate service to control the directive.

- The delegate service allows you to access and control individual instances of a directive. This allows us to have multiple instances of the same directive in the same controller.

- All the pdf controls are available through the delegate.
@PaulMougel
Copy link

👍 Any news about this pull request @sayanee? It would be pretty useful for us.

@sayanee
Copy link
Owner

sayanee commented Sep 25, 2014

Thanks @winkerVSbecks! Sorry about the delay as I was fixing the rest of the issues. Could you rebase on top of the master?

@winkerVSbecks
Copy link
Author

@sayanee @PaulMougel I'll do this over the weekend.

@PaulMougel
Copy link

Hello everyone, any news about this PR?

@winkerVSbecks
Copy link
Author

@PaulMougel I tried rebasing but, there were just too many changes.

Also, in my opinion it makes sense to use the PDFJS as a delegate and there isn't really any need to replicate those methods in angular. Therefore, I'm making a few changes and will release it as a different library in a couple of days.

@sayanee
Copy link
Owner

sayanee commented Oct 23, 2014

👍 @winkerVSbecks sounds good to release it as a separate library

@PaulMougel
Copy link

Thanks for the quick answers, keep us updated.

@winkerVSbecks
Copy link
Author

@PaulMougel here you go https://github.com/winkerVSbecks/angular-pdf-viewer Let me know if you come across any bugs or if you have any feature requests.

@sayanee sayanee closed this in 81ffee7 Oct 30, 2014
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.

3 participants