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

Email design update #746

Merged
merged 11 commits into from Apr 9, 2018

Conversation

Projects
None yet
5 participants
@jakerockland
Copy link
Contributor

commented Mar 30, 2018

Description

This PR is a current work in progress solution for updating the email templates as described in issue #563.

Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)

n/a?

Testing

I have only changed HTML/CSS for email templates (all Django templating has been left unchanged).

Refers/Fixes

Fixes: #563

Screenshots of Current Progress

screenshot 2018-03-29 18 49 30

screenshot 2018-03-29 18 49 35

screenshot 2018-03-29 18 49 07

screenshot 2018-03-29 18 49 13

@codecov

This comment has been minimized.

Copy link

commented Mar 30, 2018

Codecov Report

Merging #746 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #746      +/-   ##
==========================================
- Coverage   33.97%   33.92%   -0.06%     
==========================================
  Files         101      101              
  Lines        5774     5784      +10     
  Branches      672      676       +4     
==========================================
  Hits         1962     1962              
- Misses       3733     3742       +9     
- Partials       79       80       +1
Impacted Files Coverage Δ
app/app/urls.py 93.33% <ø> (ø) ⬆️
app/retail/emails.py 24.02% <0%> (ø) ⬆️
...eting/management/commands/bounty_feedback_email.py 0% <0%> (ø) ⬆️

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 f912f7d...519207d. Read the comment docs.

@owocki

This comment has been minimized.

Copy link
Member

commented Mar 30, 2018

purdy excited about this.. what is still left TODO on this PR?

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Mar 31, 2018

@jakerockland Could we

  • update the fonts to Muli ?
  • Make the title bold while the text normal
  • reduce the overall padding of the box, looks like we've got a lot of breathing space within the box
  • remove the fire emoji :P and update it with the number of newly funded issues
  • last but on least -> squash the commits

PS: Does this PR also update the Gitcoin weekly mail ?

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2018

@thelostone-mc sure thing, can take care of all of those things. I like to commit early and often so was waiting to squash commits until after the WIP is a bit more finalized. 😄Also I don't believe that this PR updates the Gitcoin weekly email (unless the weekly email uses template.html). The design file for the weekly email was included as a reference in #563 but it wasn't mentioned that it was part of the included changes so I wasn't sure if I was to look into addressing that with this PR.

@owocki only outstanding things are those listed above from @thelostone-mc (I wanted to start off with WIP PR to see if there were any other outstanding tweaks changes that anyone had in feedback) and also I still need to verify if new_tip and roundup pages look like they should. I saw and appreciate your feedback in #563 on verifying that but haven't had the chance to generate tip object in the DB (new to this stack/Django in general) and adjust parameters for roundup testing.

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Apr 1, 2018

Ah other than that , everything else looks solid :D
Let's get this PR merged once everything is addressed \m/

@owocki

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

screenshots of the tip / roundup emails:

screencapture-localhost-8080-_administration-email-new_tip-2018-04-02-15_40_05
screencapture-localhost-8080-_administration-email-roundup-2018-04-02-15_40_29

@owocki

This comment has been minimized.

Copy link
Member

commented Apr 2, 2018

@owocki only outstanding things are those listed above from @thelostone-mc (I wanted to start off with WIP PR to see if there were any other outstanding tweaks changes that anyone had in feedback)

makes sense to me!

also I still need to verify if new_tip and roundup pages look like they should. I saw and appreciate your feedback in #563 on verifying that but haven't had the chance to generate tip object in the DB (new to this stack/Django in general) and adjust parameters for roundup testing.

i posted some screenshots of this above. tip email looks okay. roundup email needs a little work though

thanks... looking forward to getting this to merge!

@PixelantDesign

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

Looking good @jakerockland!

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

@owocki The redeem tip button!
Thoughts on putting it above the logo + make it a big fat button to grab the users attention ?

@owocki

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

Thoughts on putting it above the logo + make it a big fat button to grab the users attention ?

I agree! @jakerockland can you make it so!?

roundup email needs a little work though

i think this could use another pass too. you can edit the source data in retail/emails.py if you are still having problems with the bounties not existing in your local DB

@owocki

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

@jakerockland once we get the merge conflicts resolved and resolve the above issues im looking forward to merging this!

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

commented Apr 4, 2018

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2018

So I believe that this PR addresses almost all outstanding issues now except for one thing, on the single bounty page, are we sure we want to remove the fire emojis? From what I understand, that email currently will only ever have content for one bounty.

@owocki I was able to get things working with being able to effectively work on roundup and new_tip. Work on the roundup email is taking more time than I expected but should be able to finish things up tomorrow.

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@thelostone-mc Is it possible to merge this with Githubs squash & merge feature? I'm having trouble squashing my commits now that I've pulled the upstream changes to resolve the merge conflicts from adding translation support.

Also I was able to cleanup the roundup email so I think this may be close to being ready to merge?

@thelostone-mc

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

@jakerockland You won't have merge conflicts in this case but it's always good to rebase before getting the PR is merged!

That said, let's get this merged and celebrate 🍻

Squash dem commits
  • git rebase -i HEAD~10 # this will list last 10 commits
  • Leaving the first one , reword pick to f # this says squash(fixup) last 9 commits

screen shot 2018-04-09 at 6 32 59 pm

  • git log and verify there is only one commit and check your changes
  • git push origin email-designs -f # will rewrite the history on github
Rebase with upstream :
  • git fetch upstream master
  • git rebase upstream master
  • git push origin email-designs -f

That should do the trick ^_^

@mbeacom

mbeacom approved these changes Apr 9, 2018

Copy link
Contributor

left a comment

lgtm. @owocki Are you good with this PR?

@mbeacom mbeacom changed the title WIP: Email design update Email design update Apr 9, 2018

@owocki

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

519207d <= there were some untested emails.. i just submitted some fixes for them here.. now i'm good with the merge

shipit! :shipit:

@jakerockland

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@thelostone-mc So the problem is that there were previously merge conflicts with the master branch so I pulled the master branch into this branch to resolve those conflicts, but now when I try to rebase using the above method, the commit history includes all of the commits that were on master that I pulled into my branch, not just my commits onto this branch before pulling master in. 😓

@mbeacom

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

@jakerockland Don't worry about rebasing this branch. I'll squash commits when I merge it.

@mbeacom mbeacom merged commit 0e22100 into gitcoinco:master Apr 9, 2018

3 of 5 checks passed

codecov/patch 0% of diff hit (target 33.97%)
Details
codecov/project 33.92% (-0.06%) compared to f912f7d
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jakerockland jakerockland deleted the jakerockland:email-designs branch Apr 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.