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

Add higher dpi images for warning and info email icons #11566

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

shanechesnutt-ft
Copy link
Contributor

🛠 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

Before After
old_warning new_warning
old_info new_info

Copy link
Member

@aduth aduth left a 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), keeping info.png as-is
  • Disregard whether it's broadly supported, since it's only used for non-production testing reports anyways (internal use only?)

@aduth
Copy link
Member

aduth commented Nov 26, 2024

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.

@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/fix_email_warning_and_info_icons branch from a4f1cf4 to 59679c8 Compare November 27, 2024 14:40
Copy link

@rutvigupta-design rutvigupta-design left a 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!

@gina-yamada
Copy link
Contributor

gina-yamada commented Nov 27, 2024

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

Copy link
Contributor

@eileen-nava eileen-nava left a 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.

@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/fix_email_warning_and_info_icons branch from 59679c8 to d457429 Compare December 5, 2024 14:17
@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/fix_email_warning_and_info_icons branch from d457429 to a3aa8d2 Compare December 17, 2024 20:21
changelog: User-facing Improvements, In-person Proofing, Add higher dpi images for warning and info alert email icons
@shanechesnutt-ft shanechesnutt-ft force-pushed the sc/fix_email_warning_and_info_icons branch from a3aa8d2 to 92a26a8 Compare December 19, 2024 15:16
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.

5 participants