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 native Prawn template #27

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

800a7b32
Copy link

@800a7b32 800a7b32 commented Jun 6, 2022

This PR provides native prawn templates for the QR-Bills gem. The prawn block is wrapped in #canvas as per prawn specs

@damoiser
Copy link
Owner

damoiser commented Jun 6, 2022

Hello @800a7b32, thanks for the PR! Seen that this is a bit important changes, I would require to check deeper your code changes, will do that as soon I will have some spare time and provide feedback on the code.

Thanks again!

Copy link
Owner

@damoiser damoiser left a comment

Choose a reason for hiding this comment

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

Thanks again, see my feedback, will do a test for the prawn creation and let u know

require 'qr-bills/qr-creditor-reference'

module QRBills
def self.generate(qr_params)
def self.generate(qr_params, pdf = nil)
Copy link
Owner

Choose a reason for hiding this comment

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

thanks for the PR! I would find better to pass an additional params directly in the qr_params variable, you can add a new parameter there called [:output][:prawn][:pdf], so we can leave all prawn objects under that place. Or eventually we can create a new section for the inputs like "[:format_inputs][:prawn][:pdf]"

rqrcode (>= 2.1, < 3)

GEM
remote: https://rubygems.org/
specs:
byebug (11.1.3)
Copy link
Owner

Choose a reason for hiding this comment

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

why was this removed?

format("%s<br>%s<br>%s<br>", address[:name], address[:line1], address[:line2])
end
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

thanks for this, I would need to test it accordingly

@@ -54,15 +54,18 @@

it "fails if currency type is empty" do
@params[:bill_params][:currency] = ""
@params[:bill_type] = QRParams::QR_BILL_WITH_QR_REFERENCE
Copy link
Owner

Choose a reason for hiding this comment

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

why this and the following rows are been added?

@danielpuglisi
Copy link
Contributor

If I may add some input: I would recommend replacing QRPRAWNLayout with a prawn extension module like so https://prawnpdf.org/docs/0.11.1/Prawn/Document.html#method-c-extensions (e.g. call it QRBills::PrawnExtension).

With this there is no need for additional params and layout logic and one can just simply add the extension and use it within their pdf workflow.

An example:

  Prawn::Document.extensions << QRBills::PrawnExtension

  Prawn::Document.generate("foo.pdf") do
    qr_bills_payslip(qr_bills_params)
  end

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.

4 participants