-
Notifications
You must be signed in to change notification settings - Fork 116
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
Add higher dpi images for warning and info email icons #11566
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One potential incompatibility here is with the tables reports mailers. It looks fine in the browser preview, but it relies on background-size
to size the icon, which may not be broadly supported in common email clients:
https://www.caniemail.com/search/?s=background-size
Mailer example: http://localhost:3000/rails/mailers/report_mailer/fraud_metrics_report
A couple ideas, in order of preference:
- Update the tables report to use the inlined
<img>
like we're doing in other mailers - Create a copy of the image at higher resolution to use when inlining element (e.g.
info@4x.png
), keepinginfo.png
as-is - Disregard whether it's broadly supported, since it's only used for non-production testing reports anyways (internal use only?)
I probably should have started with saying that this is a great change, happy to see it 👍 😄 As mentioned in Slack, we probably want to run these images through an optimizer like Squoosh to offset the potential file size increase. |
a4f1cf4
to
59679c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved from design, this looks so much better! 🎉 I'm curious about the considerations Andrew raised, so please let me know if design can support with anything else!
I was digging around to see affects of change in background-image. I see that Andrew pointed out some concerns with this. Per his suggestion, it looks like you may need to rework the tables report a bit. Let me know if you need anything else reach out when this is ready for another review. TIL about http://localhost:3000/rails/mailers/report_mailer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this, the images definitely look better with higher dpi. I'm going to hold off approving, based on feedback from Andrew. Let me know if I can help with continued work on this task.
59679c8
to
d457429
Compare
d457429
to
a3aa8d2
Compare
changelog: User-facing Improvements, In-person Proofing, Add higher dpi images for warning and info alert email icons
a3aa8d2
to
92a26a8
Compare
🛠 Summary of changes
Replaced low resolution/dpi pngs with higher resolution/dpi pngs to improve look and feel of warning and info icons in emails.
👀 Screenshots