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 responsive email format to the babylon-notifier #662

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

makirill
Copy link
Contributor

@makirill makirill commented Nov 3, 2022

MJML is a framework to generate responsive emails (more details at https://mjml.io/)

There are two major changes needed to introduce MJML framework to the babylon-notifier:

  1. mjml cli needs to be installed, so it can be used by the operator.py script
  2. Jinja2 templates should generate valid mjml format that will be used to generate final HTML for the email notifications.

I did the "success" path testing (provisioning start -> environment ready -> environment deleted). Deployment was done with odo tool.

<mj-section padding-top="21px" padding-bottom="12px" padding-left="12px" padding-right="12px">
<mj-column>
<mj-text font-family="Red Hat Text" font-weight="regular" align="center" font-size="16px" color="#000" line-height="1.2">
Status: Service Ready
Copy link
Contributor

@aleixhub aleixhub Nov 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be better placed under the content block? or a variable? If I understand correctly all the emails will contain the Service Ready even though they might belong to a provision failed alert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I've fixed it with 75d1a3b

@makirill makirill marked this pull request as ready for review November 8, 2022 12:56
<mj-section padding-top="12px" padding-bottom="12px" padding-left="12px" padding-right="12px" background-color="#f0f0f0">
<mj-column>
<mj-text font-family="Red Hat Text" color="#4a5557" font-size="16px" line-height="1.5" padding-bottom="12px">
{% block content %}{% endblock %}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Lift the content to be inside the mj-section
And then in every .mjml template use the mj-column / mj-text accordingly. As it is now the templates will not have lots of flexibility as they are encapsulated under those elements and they use plain html syntax without having the benefits of mjml syntax

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. I wanted to implement it this way initially but for some reason I've change my mind and left child templates with pure HTML/JINJA2 without any MJML tags.

New commit should fix it - 164ccd9

Copy link
Contributor

@aleixhub aleixhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great work

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.

None yet

2 participants