Pull in old png Solidus logo #20
Conversation
There was a problem hiding this comment.
Hey @Sinetheta, thanks a lot for this PR! 🎉
Just one thing I'd like you to fix before approval: can you get the change from lib/solidus_print_invoice/version out of this PR? A new release is definitely coming, but we need to do some extra maintenance before doing so.
Thanks once again! 🙌
The Solidus admin area was recently changed to use an svg logo solidusio/solidus#2995 but that is not supported by Prawn so it broke the pdf embed here. The old png logo is no longer used in Solidus so it's only a matter of time before someone removes it. As such I have pulled it in for use in the pdf header, but kept the same name because I figured it less likely that the png logo ever be updated again in Solidus than maybe someone customized their Solidus install by overwriting the image instead of changing the config. This has the unfortunate affect of no longer automatically following Solidus admin logo customizations, but I don't think it would be worth doing fancy svg detection at that point. We have our own config, folks can use that if needed.
|
No problem @aitbw I was just taking a stab at greening up https://extensions.solidus.io/ 😆 |
|
preference :print_invoice_logo_path, :string, :default => Spree::Config[:print_invoice_logo]Since it is already a preference, probably there's no need |
|
@Sinetheta discussing this with @mdesantis, @spaghetticode, and @kennyadsl, I feel like we can improve this PR a little more: what do you think about a conditional to use the preference (if it's a PNG) or use the Solidus logo as a fallback —that way we won't break invoices with custom logos. |
|
Superseeded by #22. Thanks for your contribution anyway, @Sinetheta. Much appreciated. 🙇♂️ |
|
👍 thanks guys, just happy to see a fix went it. Sorry I didn't see the notifications. |
This PR makes us green with Solidus master.
The Solidus admin area was recently changed to use an svg logo solidusio/solidus#2995 but that is not supported by Prawn so it broke the pdf embed here.
The old png logo is no longer used in Solidus so it's only a matter of time before someone removes it. As such I have pulled it in for use in the pdf header. I kept the same filename because I figured it less likely that the png logo ever be updated again in Solidus than maybe someone customized their Solidus install by overwriting the image instead of changing the config.
This has the unfortunate affect of no longer automatically following Solidus admin logo customizations, but I don't think it would be worth doing fancy svg detection at that point. We have our own config, folks can use that if needed.