chore(docs): Update docs around emails #197
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LZoog
force-pushed
the
issues/10758
branch
3 times, most recently
from
cac1ac7 to
b286eae
Compare
xlisachan
approved these changes
Apr 11, 2022
docs/reference/emails.md
Outdated
| Emails previewed in HTML are meant to be a rough representation of what an email will look like in an email client. They're essentially identical and MJML helps us with consistency, but keep in mind you're previewing in a browser when emails may be viewed in email clients in practice. | ||
| ::: | ||
|
|
||
| We couple Storybook with the `merge-ftl` grunttask explained in the l10n section to display strings from our `en.ftl` files. At the time of writing, Storybook is our way to preview or manually check the English strings we ultimately pass to translators, and our tests cover the English fallback copy. |
There was a problem hiding this comment.
Nit (ln 85): We couple Storybook with the merge-ftl grunttask to display strings from our en.ftl files (see the l10n section for more details).
| ## Templates | ||
|
|
||
| FxA emails use [EJS](https://ejs.co/) to allow logic and conditional rendering without additional helper methods, [MJML](https://mjml.io/) to shift the burden of maintaining solutions for email quirks off of FxA engineers, [SCSS](https://sass-lang.com/) with [Tailwind](https://tailwindcss.com/)-like classes compiled down to inline CSS for easy maintenance and consistency, and [Fluent](https://github.com/projectfluent/fluent.js/tree/master/fluent-dom) for localization. We also create [Storybook](https://storybook.js.org/) stories to preview emails and for documentation purposes. [See this ADR](https://github.com/mozilla/fxa/blob/main/docs/adr/0024-upgrade-templating-toolset-of-auth-server-emails.md) for more detail on why this stack was chosen. | ||
|
|
There was a problem hiding this comment.
Last sentence: detail -> details
| We couple Storybook with the `merge-ftl` grunttask explained in the l10n section to display strings from our `en.ftl` files. At the time of writing, Storybook is our way to preview or manually check the English strings we ultimately pass to translators, and our tests cover the English fallback copy. | ||
|
|
||
| In addition to running locally, Storybook can be built into a static format. This is ultimately what happens in our CI. This static format is then deployed and manual QA may be conducted against it. To test what this will look like, run `yarn build-storybook` then navigate to the `storybook-static` folder and run `http-server . -p 8081`. From there, simply navigate to the `http://locahost:8081/index.html` and you will see the resulting static build. _(If you don't have `http-server` installed, simply run `npm install -g http-server`)_ | ||
|
|
There was a problem hiding this comment.
Nit: remove the from last sentence - simply navigate to http://locahost:8081/index.html ...
|
|
||
| :::info | ||
| The previews in Storybook are generated using the `mjml-browser` module and not the de facto `mjml` module. The difference is subtle but important. As the name indicates, `mjml-browser` is designed to render from a browser context, whereas mjml uses a nodejs context. Ideally these modules would be identical, but at the time of writing this, there are a couple [minor differences](https://github.com/mjmlio/mjml/tree/master/packages/mjml-browser#unavailable-features). Our code introduces a couple work arounds to achieve parity with the way templates would be rendered using the `mjml` module. If styles in sent out emails ever seem off, consider this discrepancy as an unlikely but potential source of error. | ||
| ::: |
There was a problem hiding this comment.
Nit: couple of [minor differences]
| ``` | ||
|
|
||
| We could have technically localized strings this way as well rather than wrap text elements in `span`s, but that would have been significantly messier and confusing. | ||
|
|
There was a problem hiding this comment.
We should add in a reminder that if no width is provided in the mj-image tag that the image will use the parent column width, as we saw in this PR - mozilla/fxa#11840 😅
There was a problem hiding this comment.
Thanks for calling this out, I definitely forgot this piece. I added a note to the top of this section about it with a link to your PR, as well as added a comment within the MJML image example.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The original ticket covers updating the Localization docs, but I need to create a separate ticket for that, or I'll just take #116. I had to add a lot of emails-specific l10n info and some of that can carry over into that ticket. The two l10n paragraphs from this PR are repeated there but I might end up pulling that into the Localization page.
Please feel free to call out whatever is causing weird formatting in the PR. I know there's a mix of straight vs curly quotes so maybe that's causing it, but it looks fine even in MD preview to me.
closes mozilla/fxa/issues/10758