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

Redesign Weekly Email #6438

Merged
merged 31 commits into from
Apr 29, 2020
Merged

Conversation

sebastiantf
Copy link
Contributor

@sebastiantf sebastiantf commented Apr 15, 2020

Description

This PR updates the Weekly Email template according to the new layout provided here at #6208

Note: Some dummy data for the email content has been added in emails.py. It can be used to refer the modifications made to the template in this PR. But the dummy data shall have to be excluded from production.

Note: But the kudos_friday and hide_bottom_logo params lines will have to be included.

Refers/Fixes

Fixes #6208

Screenshots

Laptop:

weekly_final1_laptop

Mobile:

weekly_final1_mobile

@codecov
Copy link

codecov bot commented Apr 15, 2020

Codecov Report

Merging #6438 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6438   +/-   ##
=======================================
  Coverage   27.09%   27.09%           
=======================================
  Files         291      291           
  Lines       26682    26682           
  Branches     3948     3948           
=======================================
  Hits         7229     7229           
  Misses      19186    19186           
  Partials      267      267           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d35c2d3...d35c2d3. Read the comment docs.

Copy link
Member

@thelostone-mc thelostone-mc left a comment

Choose a reason for hiding this comment

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

Could you add it in :

  • screenshots (mobile + laptop)
  • could you remove out commented out code unless it's needed for some reason

@owocki
Copy link
Contributor

owocki commented Apr 15, 2020

thanks. looks pretty good! i hope it renders well across mail clients. i know that mail client availability is kind of a sticking point

@sebastiantf
Copy link
Contributor Author

@thelostone-mc I have removed the commented code and have also included the screenshots in the opening comment.

Should I do anything else?

@sebastiantf sebastiantf force-pushed the redesign-weekly-email branch 2 times, most recently from 51610a5 to 3a26f48 Compare April 16, 2020 16:28
@sebastiantf
Copy link
Contributor Author

sebastiantf commented Apr 21, 2020

@sebastiantf Please rebase this on the latest master so you can see the recent changes to how the content for this email is loaded

@danlipert Even after rebasing onto the latest master, I couldn't find any changes regarding how the content for this email is loaded. Am I missing something? Did you only mean the original way of fetching the content that was used before I entered the dummy data?:

    args = RoundupEmail.objects.order_by('created_on').last()
    .........
    intro = args.body.replace('KUDOS_INPUT_HERE', kudos_friday)
    highlights = args.highlights
    sponsor = args.sponsor
    bounties_spec = args.bounties_spec

@sebastiantf
Copy link
Contributor Author

@danlipert I have rebased onto the latest master and also removed the dummy code. Please review.

@thelostone-mc thelostone-mc requested a review from danlipert April 22, 2020 06:32
Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

I loaded this in my local and it had some obvious design issues and the kudos section is duplicated. How was this tested? I think using the dummy data instead of getting it from the Marketing > RoundupEmail model may have given some false confidence

@@ -1085,11 +1085,13 @@ def render_new_bounty_roundup(to_email):
'invert_footer': False,
'hide_header': False,
'highlights': highlights,
'kudos_friday': kudos_friday,
Copy link
Contributor

Choose a reason for hiding this comment

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

Kudos already get inserted into the intro, which is rendered into the template as part of the intro block (see Line 1028 in this file, Line 89 in the template) - correct? Don't the kudos show up twice?

Copy link
Contributor Author

@sebastiantf sebastiantf Apr 22, 2020

Choose a reason for hiding this comment

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

I wasn't aware that the kudos is inserted into the intro. I'd understood that only the textual content is what's loaded into the intro. It would be helpful if you could provide a sample email content that would typically be entered in the RoundupEmail model.

Copy link
Contributor Author

@sebastiantf sebastiantf Apr 22, 2020

Choose a reason for hiding this comment

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

I shall have removed the code that passes and inserts the kudos_friday again into the template.

@sebastiantf
Copy link
Contributor Author

sebastiantf commented Apr 22, 2020

@danlipert If you could provide the content of the (or any) sample email that you used in the RoundupEmail for testing it would be helpful, I guess. I don't have a sample email in the dev environment.

I'd tested it using the dummy data that I inserted in commit 794aed9c2ab8c4eb03260e6c0e77f2a6f17e1360

@sebastiantf
Copy link
Contributor Author

@danlipert One of the design issues that you may have seen could be because I've used a class .regards in the intro block to identify the email author image at the end of the intro block. Since the image had to be rounded and the name inline, I had to target it and adding a class into that part is what I thought of.

Please see these lines of:

@sebastiantf sebastiantf requested a review from danlipert April 22, 2020 14:04
@danlipert
Copy link
Contributor

This looks pretty good - I'm going to merge it @sebastiantf but can you please do a followup PR to fix the kudos cards layout? check this screenshot from production:
screencapture-localhost-8000-administration-email-roundup-2020-04-29-21_33_59

@danlipert danlipert merged commit aeca879 into gitcoinco:master Apr 29, 2020
@danlipert
Copy link
Contributor

@sebastiantf we're deploying it today so please do a followup PR

@sebastiantf
Copy link
Contributor Author

sebastiantf commented Apr 29, 2020

Ok @danlipert.

And also, I see in the screenshot you've posted above that the author image at the end of the intro is still in full size. I had defined a .regards class to target that image and make it rounded.

You'll have to use a div with .regards class in the intro around the image to make it rounded.
You may see this for reference: div .regards

@sebastiantf
Copy link
Contributor Author

@danlipert Please see #6526. I have the kudos aligned.

thelostone-mc added a commit that referenced this pull request Apr 29, 2020
@sebastiantf sebastiantf mentioned this pull request Apr 30, 2020
@sebastiantf
Copy link
Contributor Author

sebastiantf commented Apr 30, 2020

@danlipert Did you try generating an email from this PR? It seems there are issues with the email not being rendered properly on email clients due to a lack of support.

This is what I got when generating an email and viewing it on Gmail:

[image moved to #6534]

The width is actually too much which may not be clear from the above screenshot. Scrollbars are showing up due to the width being too much:

[image moved to #6534]

I am creating a WIP PR solving these issues: #6534
I shall make it ready after fixing these and many other issues.

@danlipert
Copy link
Contributor

@sebastiantf sounds good, appreciate it - I did not try testing it via sending an email, just took a look at the in-browser preview url at localhost:8000/_administration/email/roundup

@sebastiantf
Copy link
Contributor Author

sebastiantf commented Apr 30, 2020

@danlipert #6534 is ready for review.

I see that the new version was released about an hour ago. I'm not sure how you send out the emails, but I suggest sending out the weekly emails only after applying the changes in #6534 and testing it out.

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.

Update weekly email build
5 participants