-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Autogenerate text emails in CE #4674
base: master
Are you sure you want to change the base?
Conversation
59c1962
to
e6d2663
Compare
lib/plausible_web/email.ex
Outdated
{"href", href} = List.keyfind!(attrs, "href", 0) | ||
text = Floki.text(children) | ||
text_and_link = IO.iodata_to_binary([text, " (", href, ?)]) | ||
{"p", attrs, [text_and_link]} |
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.
Some examples of how it gets rendered in text:
Click here to upgrade your subscription (http://localhost:8000/billing/choose-plan).
In your account settings (http://localhost:8000/settings/billing/subscription), you'll find an overview of your usage and limits.
703adc1
to
b57ed92
Compare
b57ed92
to
e044531
Compare
defp textify(html) do | ||
html | ||
|> Floki.parse_document!() | ||
|> Floki.traverse_and_update(&textify_link/1) |
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.
haha clever. any reason not to do it on EE?
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 want to make EE emails slower :) And EE probably doesn't need it since Postmark might be doing something similar already.
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.
Ah, no. Postmark only sends HTML (based on an email from staging). But then, I guess it's still OK since we only had problems with CE users.
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 want to make EE emails slower :)
Is there a significant slow down? If so, I'm sure you'd be tempted to generate those text versions statically with a mix task instead or come up with something even more clever ;D
I think including text versions boosts our reputation? Either way, even Postmark recommends it https://postmarkapp.com/guides/transactional-email-best-practices#11-include-well-formatted-plain-text-versions so I really don't think we should be that pedantic about build separation here. cc @ukutaht
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 wanted to sort of pre-compile the text templates into functions like EEx does, but then got paranoid about dynamic parts staying HTML ... I can look into it more. I'll also run some benchmarks to see how much of a performance hit the current "textifying" approach is.
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.
haha, ace!
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.
Nice work @ruslandoga!
Yeah I agree with @aerosol, adding text emails to EE will be useful I think. And I don't have any performance concerns here. Maybe I'm overconfident but I don't see how email formatting would be a bottleneck in any way.
@@ -1,6 +1,5 @@ | |||
Your <%= Plausible.product_name() %> export for <%= @site.domain %> is now ready for download. | |||
Please click <a href={@download_url}>here</a> | |||
to start the download process. | |||
<p>Please click <a href={@download_url}>here</a> to start the download process.</p> |
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.
Seems like Floki.text
preserves whitespace within elements, as if like <pre>. So to avoid too much whitespace from the HEEX+Phoenix.LiveView.HTMLFormatter, HTML email templates might need to be updated to use p/span tags, and not just plain text. Or by ignoring all newlines Floki returns except for <br> tags.
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.
Without this p tag change, in the text version, this email is rendered as
Hey Jane,
Your Plausible CE export for example.com is now ready for download.
Please click here (http://localhost:8000/example.com/download/export)
to start the download process.
--
http://localhost:8000
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.
And after wrapping the line in a p tag
Hey Jane,
Your Plausible CE export for example.com is now ready for download.
Please click here (http://localhost:8000/example.com/download/export) to start the download process.
--
http://localhost:8000
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 think the ideal version would be
Hey Jane,
Your Plausible CE export for example.com is now ready for download.
Please click here (http://localhost:8000/example.com/download/export) to start the download process.
--
http://localhost:8000
This can probably be achieved by ignoring all newlines except for <br>
Changes
This PR autogenerates text versions of non-MJML emails in CE.
Closes #4249
Tests
Changelog
Documentation
Dark mode