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

Optional combine_pdf + support yarn pnp #266

Merged
merged 9 commits into from
Nov 19, 2024

Conversation

le0pard
Copy link
Contributor

@le0pard le0pard commented Nov 6, 2024

Reason for this changes

Gem combine_pdf needed only for middleware and only for front_cover and/or back_cover functionality. This mean, that people which use grover directly by display_url or raw html need to have this gem as additional dependency. Problem, that combine_pdf contain as deps https://github.com/caiges/Ruby-RC4 , which is not supported any more (dead dependency).

https://github.com/boazsegev/combine_pdf?tab=readme-ov-file#unmaintained---help-wanted - combine_pdf also unmaintained

Maintained solutions to use pdfunite cli from poppler library or to use https://hexapdf.gettalong.org/examples/merging.html (paid solution)

Screenshot 2024-11-06 at 02 25 37

By this proposal I making combine_pdf as optional and not install as dependency, so developers, which need middleware and front_cover and/or back_cover functionality can install it separately in application.

Also added ability to override js runtime bin, so in this case devs, which use non standard env, like yarn pnp, can override hardcoded node bin (example added in README). Possible fix for #262

Copy link
Contributor

@abrom abrom left a comment

Choose a reason for hiding this comment

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

Nice one @le0pard 👍

I've got a few suggestions on some copy improvements, but otherwise looking solid

README.md Outdated Show resolved Hide resolved
lib/grover/middleware.rb Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
le0pard and others added 5 commits November 18, 2024 11:00
Co-authored-by: Andrew Bromwich <[email protected]>
Co-authored-by: Andrew Bromwich <[email protected]>
Co-authored-by: Andrew Bromwich <[email protected]>
Co-authored-by: Andrew Bromwich <[email protected]>
@le0pard le0pard requested a review from abrom November 18, 2024 09:03
@le0pard
Copy link
Contributor Author

le0pard commented Nov 18, 2024

Thanks @abrom , fixed

Copy link
Contributor

@abrom abrom left a comment

Choose a reason for hiding this comment

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

Fantastic, thanks @le0pard 👍

@abrom abrom merged commit 3e72dd2 into Studiosity:main Nov 19, 2024
11 of 12 checks passed
@abrom
Copy link
Contributor

abrom commented Nov 19, 2024

Released in v1.2.0

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