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

Support For Multiple Embeds #29

Closed
wants to merge 4 commits into from
Closed

Support For Multiple Embeds #29

wants to merge 4 commits into from

Conversation

chtzvt
Copy link
Contributor

@chtzvt chtzvt commented Jun 5, 2022

Hello all,

This PR adds support for multiple embeds as discussed in #25. I've rolled the changes I've made for this into the original shortcode, and added a new partial responsible for loading pdf.js and the shortcode styles.

Give it a test and let me know what you think.

…e instance's elements.

Migrates JS initialization of the viewer to an IIFE rather than using window.onload.

Moves pagination controls underneath the PDF image and adds a link to the original document. This link will also appear for users with JavaScript disabled.
@sonarcloud
Copy link

sonarcloud bot commented Jun 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@chtzvt
Copy link
Contributor Author

chtzvt commented Jun 9, 2022

Note that out of necessity, this PR changes how the PDF.js library is loaded.

As such, particular consideration should be taken with respect to #19. It may be wise to add a flag in the Hugo configuration where a site owner could specify the location of their own PDF.js instance, or fetch it from a CDN otherwise inside of the dependency loader.

@chtzvt
Copy link
Contributor Author

chtzvt commented Jul 21, 2022

@anvithks Any thoughts on this?

@anvithks
Copy link
Owner

@anvithks Any thoughts on this?

I am sorry but I haven't been able to test this yet.
I will try to spend some time over the weekend and merge the PR if all OK.

Thanks @ctrezevant .

@RoneoOrg
Copy link
Contributor

Hi @ctrezevant , thanks for sharing this code!

PR #32 is a proposal mixing your code and this little trick: splitting the code into multiple files can be avoided. See 0b75911 and this discussion on discourse.gohugo.io.

I've tested #32 and it works fine. If it's not working for you, please double-check the commits f7a8fec and e3642f0. They are required to make this shortcode work with my theme (Papermod) but your setup may be different

@chtzvt
Copy link
Contributor Author

chtzvt commented Sep 30, 2022

Glad to see this moving forward! Everything looks solid after a cursory review.

@sonarcloud
Copy link

sonarcloud bot commented Mar 15, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@anvithks
Copy link
Owner

@chtzvt Closing this as it was added to #32 and merged with your commits.

@anvithks anvithks closed this Mar 15, 2023
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