-
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
System emails design #4818
System emails design #4818
Conversation
The failing spec it seems not related with these changes. 🤔 |
That's right, that spec has been failing all day... maybe there's an update in chrome/chromedriver causing it. We'll take a look next week! |
Hi! |
Hi @microweb10, I think we fixed it in: |
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.
@taitus I've left some comments; let me know what you think!
Currently with both seeds and dev_seeds, not only was this email not displayed from the system emails section, but it also caused an error in the application. @email_to had an empty value and in the view we tried to access @email_to.name which caused the error. We kept the same logic but added the current_user to make sure it always has a valid value. We add the current_user because the current_user is always present in this controller..
The notification digest title did not look the same as other mail. We removed the table for the title to make it more consistent with the rest of the emails.
In the comments and direct message emails, the "attributes" was missing and in the reply email it was not in the right place.
This section was being displayed differently in each mailing. We are trying to unify it so that they are displayed the same.
expect(page).to have_content comment.body | ||
|
||
expect(page).to have_link "Cleaner city", | ||
href: admin_budget_budget_investment_url(investment.budget, investment, anchor: "comments", host: app_host) |
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.
Layout/LineLength: Line is too long. [117/110] (https://rubystyle.guide#max-line-length)
In the mail section we have very different indentations and formatting in texts with sanitize, links and texts with interpolations. In my opinion it helps a lot to have clearer indentations in these cases. This may not be the best way to indent them, but at least I think it is clearer than it was and at least relatively unified.
References
In the System Emails section there are 12 emails that are sent from CONSUL.
As the styles for these emails must be inline, it's complicated to modify them to add custom styles. Also, as the emails were created at different times there were some inconsistencies in the design.
Objectives
This PR adds the following changes:
Add mailer helpers to simplify customization: all styles can now be changed from the helper and are common to all emails, maintaining the design.
Remove icon images on mailers: these images do not contribute anything to the user and are not used on the rest of the website.
Add missing heading title on mailers: for consistency, titles have been added to all emails that did not have one.
Visual Changes 👨🎨