Closed danielpuglisi closed 2 years ago
thx for the PR! I would need to do some more deep checks on the svg changes, in the meantime, can you please check my feedback and fix the merge conflicts?
Merge conflicts are fixed. I don't see any other feedback regarding this PR. Did you add comments somewhere that I might have missed?
Regarding the code changes: There are some potential backwards incompatibilities if existing users call the following methods directly in their code base:
QRGenerator.create
does not accept the entire QRParams hash anymore, just the params[:bill_params] as that's the only data needed to create the QR code.QRHTMLLayout.html_layout
now requires a new parameter qrcode_data_url
.sorry forget to publish the feedbacks, you should see now @danielpuglisi
Latest commit addresses your comments:
thx a lot for the great PR @danielpuglisi !
You're welcome, thx for merging 🙏😊
This PR adds SVG support (#22).
To support both SVG and PNG during the HTML layout generation I added image data url support: https://github.com/damoiser/qr-bills/compare/master...codegestalt:svg-support?expand=1#diff-1707220acf1a40ad0f84d3991ef6d6f567c0b9b63b390548a0ef7e1047d17989R11
The parameter
qrcode_filepath
has been removed. Using base64 encryption we don't need to touch the file system for the png format anymore and can use the existing layout without having to add additional format logic.The parameter
qrcode_format
has been added. As when generating HTML withparams[:output_params][:format]
there was no way of specifying the qr code format.params[:qrcode_format]
solves this. Although I think this is a bit smelly. ImoQRBills.generate
should just focus on generating the entire HTML payslip and remove support for creatingpng
/svg
qr codes and leave that logic toQRGenerator.create
. I would even go so far as to removeQRBills.generate
entirely and tell people to useQRHTMLayout.create
for generating an entire payslip andQRGenerator.create
for just qr codes.