-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: Email broken styles #927
Conversation
templates/base.html
Outdated
@@ -6,286 +6,170 @@ | |||
rel="stylesheet" | |||
href="https://fonts.googleapis.com/css2?family=Poppins:wght@100;300;400;500;600;700;800;900&family=Source+Code+Pro:wght@200;300;400;500;600;700;800;900&display=swap" | |||
/> | |||
<style> |
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.
Style tags bad
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.
inline styles good
templates/base.html
Outdated
</head> | ||
<body> | ||
<header> | ||
<svg |
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.
svgs bad
templates/base.html
Outdated
</tr> | ||
<tr style="height: 25px" /> | ||
<tr style="text-align: center"> | ||
<!-- these svgs don't work, to get this looking right we'd need to get PNGs served by CDN, perhaps we add these links to our app footer? --> |
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.
Coming back to these in the future
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.
fine to send em for now, SVGs do work on a few email clients (just not popular ones)
templates/failed-payment.html
Outdated
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.
didn't change any copy, just inlined styles and adjusted tags/layouts where necessary.
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #927 +/- ##
=======================================
Coverage 97.97% 97.97%
=======================================
Files 446 446
Lines 35666 35666
=======================================
Hits 34942 34942
Misses 724 724
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. 📢 Thoughts on this report? Let us know! |
✅ All tests successful. No failed tests were found. 📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
4db9e04
to
c80b92d
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.
pouring one out for your sanity having to work with html emails 😿
templates/failed-payment.html
Outdated
color: rgb(14, 27, 41); | ||
" | ||
> | ||
Your ${{ amount }} payment to Functional Software, Inc, dba |
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.
Will amount
always be based on currency of USD $
? Just double checking
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.
No, there's an upcoming change to these formats to accommodate other currencies though, so this is fine for now.
templates/failed-payment.html
Outdated
font-weight: 600; | ||
line-height: 24px; | ||
padding: 25px; | ||
border-radius: 8px; |
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.
Just a heads up I think border-radius can sometimes not work in Outlook. I think square button is fine though
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.
I'm fine with falling back to square button yea
templates/failed-payment.html
Outdated
color: black; | ||
" | ||
> | ||
Oops! Let's fix this, {{ name }} |
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.
Is name always populated in our system? I could see it being unset sometimes unless we fall back to email or something if they never told us their name
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.
this is passed in by the caller. I believe it's name or username
for the owner atm.
This PR fixes the very broken email templates we made. Turns out email clients are very restrictive of what html/css you use... lol. This new email template should be better. Key differences are table layouts and inline styles + removal of a few misc unsupported CSS attributes.
See this slack thread for screenshots.
https://sentry.slack.com/archives/C04MTDH3ZQA/p1732303415357999