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
[WIP] frontend: Email redesign. #5592
Conversation
Automated message from Dropbox CLA bot @vaidap, it looks like you've already signed the Dropbox CLA. Thanks! |
} | ||
|
||
/* ------------------------------------- | ||
OTHER STYLES THAT MIGHT BE USEFUL |
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.
Do we need to put in all these extra styles?
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 ported all of Brock's changes without refactoring-- will remove the ones that are unnecessary for now.
{% block illustration %}{% endblock %} | ||
</div> | ||
<!-- START CENTERED WHITE CONTAINER --> | ||
<span class="preheader">This is preheader text. Some clients will show this text as a preview.</span> |
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 not sure whether my PR about preheaders will merge first and make this moot, but this should probably be a jinja2 template {% block preheader %}{% endblock %}
so we don't default to sending emails with placeholder preheader text.
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.
changed it to {% block preheader %}{% endblock %}
for now!
<tr> | ||
<td class="content-block"> | ||
<span class="apple-link">Kandra Labs, 112 S Park St, San Francisco CA 94107</span> | ||
<br/> Don't like these emails? <a href="#">Unsubscribe</a>. |
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.
Do we still need the unsubscribe block here if we have the {% block manage_preferences %}
above? Can we roll the Kandra Labs address into that area (maybe after the email-specific manage preferences content)?
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.
one problem i've been having with both the graphics and the manage_preferences
block is that they keep floating upwards-- see screenshots in #illustrations stream. my hacky solution right now was to remove manage_preferences
and just keep the kandra labs footer, and insert each image manually in each email, but that is far from optimal. would it be possible for you to have a look at this & see whether you can figure out what is causing this behaviour? i'm thinking this has to do with the order in which the template blocks are generated?
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.
it would make sense to make use of manage_preferences
as it is adequately put in the followup emails, i completely agree. just having some trouble!
@@ -26,6 +26,7 @@ | |||
{% endif %} | |||
|
|||
<p>Thanks,<br/>Zulip</p> | |||
<img src="{{ realm_uri }}/static/generated/footer.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.
Do we only want this footer image in this particular email, or should it be in all the emails? We might want to refactor this into a footer block in the base template.
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.
Looking good so far. I left some inline comments on specific things that stood out to me.
not sure why
which is not as readable, so i added some extra spaces between the blocks. not sure if this is something that should be fixed? |
Codecov Report
@@ Coverage Diff @@
## master #5592 +/- ##
=======================================
Coverage 88.75% 88.75%
=======================================
Files 438 438
Lines 54661 54661
Branches 2061 2061
=======================================
Hits 48514 48514
Misses 6147 6147 Continue to review full report at Codecov.
|
4c9e3db
to
a5c3dff
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.
The styling/images look really good. I put in a few comments with questions about the manage_preferences blocks you added. For all of the new manage_preferences blocks, not just the ones I explicitly commented on as probably unnecessary, it doesn't look like the context is fed an unsubscribe link, so I'm not sure the unsubscribe links in them will work.
@@ -15,3 +15,10 @@ | |||
Your friends at Zulip HQ | |||
</p> | |||
{% endblock %} | |||
|
|||
{% block manage_preferences %} |
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 feel like we don't want/need a manage preferences block on the password reset email since it is a one-off email specifically requested by the user. I did a quick search through my inbox and there wasn't an unsubscribe link in any of the password reset emails I've gotten lately.
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.
Good point, removed!
@@ -15,3 +19,10 @@ | |||
<p>If you recognize this login activity, you can archive this notice.</p> | |||
<p>Thanks,<br />Zulip Account Security</p> | |||
{% endblock %} | |||
|
|||
{% block manage_preferences %} |
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.
We probably shouldn't put an unsubscribe link here since this seems like the kind of thing that should be mandatory to send/receive, or at least we probably want this to be an "unsubscribe from new login notifications" type link/messaging if we allow unsubscribing.
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.
replaced this with a unsubscribe from new login notifications
.
</p> | ||
<p> | ||
Best,<br /> | ||
The Zulip Team | ||
</p> | ||
{% endblock %} | ||
|
||
{% block manage_preferences %} |
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.
As with password reset emails, we probably don't want an unsubscribe link here because this is a one-off user-requested email related to their account security.
Made the appropriate changes. |
85e65b5
to
21132e9
Compare
10b98a6
to
ac395ba
Compare
7be2bca
to
a255ebc
Compare
Hello @zulip/server-notifications members, this pull request was labeled with the "area: notifications" label, so you may want to check it out! |
Hello @zulip/server-notifications members, this pull request was labeled with the "area: notifications" label, so you may want to check it out! |
Jotting down remaining work to take this over the finish line:
Nice to do's, if they aren't too much work (but can also be done after the parts above are merged):
@hackerkid, thanks! |
Heads up @vaidap, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the |
Closing this in favor of its rebase #6383. |
Fixes #5113.
Current style: