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

1244 how it works #1501

Merged
merged 12 commits into from
Jun 25, 2018
Merged

Conversation

darkdarkdragon
Copy link
Contributor

WIP - finished one page of #1244

app/app/urls.py Outdated
@@ -175,6 +175,8 @@
url(r'^extension/chrome?', retail.views.browser_extension_chrome, name='browser_extension_chrome'),
url(r'^extension/firefox?', retail.views.browser_extension_firefox, name='browser_extension_firefox'),
url(r'^extension/?', retail.views.browser_extension_chrome, name='browser_extension'),
url(r'^how_it_works/funder', retail.views.how_it_works_funder, name='how_it_works_funder'),
Copy link
Member

Choose a reason for hiding this comment

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

rename -> /funder/

Copy link
Member

@thelostone-mc thelostone-mc Jun 19, 2018

Choose a reason for hiding this comment

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

replace url with path (just check in this file to know how)

app/app/urls.py Outdated
@@ -175,6 +175,8 @@
url(r'^extension/chrome?', retail.views.browser_extension_chrome, name='browser_extension_chrome'),
url(r'^extension/firefox?', retail.views.browser_extension_firefox, name='browser_extension_firefox'),
url(r'^extension/?', retail.views.browser_extension_chrome, name='browser_extension'),
url(r'^how_it_works/funder', retail.views.how_it_works_funder, name='how_it_works_funder'),
url(r'^how_it_works/contributor', retail.views.how_it_works_contributor, name='how_it_works_contributor'),
Copy link
Member

Choose a reason for hiding this comment

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

rename -> /contributor/

Copy link
Member

Choose a reason for hiding this comment

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

replace url with path

@@ -0,0 +1,134 @@
{% comment %} Copyright (C) 2017 Gitcoin Core This program is free software: you can redistribute it and/or modify it under
Copy link
Member

Choose a reason for hiding this comment

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

Could this be reindented?

</div>
</div>

<div class="testimonial-container w-100" style="height: 7px;">
Copy link
Member

@thelostone-mc thelostone-mc Jun 19, 2018

Choose a reason for hiding this comment

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

Could we rename it to and move the CSS to it's respective file?

@thelostone-mc
Copy link
Member

I know the image is text aligned center but it looks like it isn't cause of the shadow in the image.
Hence move the image a lil towards the right

screen shot 2018-06-19 at 5 02 31 pm

-left here

@codecov
Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #1501 into master will increase coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
+ Coverage   28.27%   28.27%   +<.01%     
==========================================
  Files         129      129              
  Lines        9520     9523       +3     
  Branches     1231     1231              
==========================================
+ Hits         2692     2693       +1     
- Misses       6730     6732       +2     
  Partials       98       98
Impacted Files Coverage Δ
app/app/urls.py 89.74% <ø> (ø) ⬆️
app/retail/views.py 39% <33.33%> (-0.13%) ⬇️

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 d9829d8...b57022f. Read the comment docs.

@@ -62,6 +62,22 @@ def index(request):
return TemplateResponse(request, 'index.html', context)


def how_it_works_funder(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

We could make this one view and determine the display type based on the path.

app/app/urls.py Outdated
@@ -175,6 +175,8 @@
url(r'^extension/chrome?', retail.views.browser_extension_chrome, name='browser_extension_chrome'),
url(r'^extension/firefox?', retail.views.browser_extension_firefox, name='browser_extension_firefox'),
url(r'^extension/?', retail.views.browser_extension_chrome, name='browser_extension'),
path(r'funder', retail.views.how_it_works_funder, name='how_it_works_funder'),
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail. If you're using r'', you'll want to switch this to re_path() - Otherwise, simply drop the r here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why this will fail?

Copy link
Member

Choose a reason for hiding this comment

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

@darkdarkdragon The docs example should shed some light on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thelostone-mc sorry, still don't understand. can you please point me to exact problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mbeacom can you please explain what is the problem here?

<div class="col text-center text-white how-it-works-footer">
<div class="mt-4 contributors-cta-text">{% blocktrans %}For more details,
please see repo maintainer's guide or the
<a target="_blank" href="https://github.com/gitcoinco/web/blob/contributing/CONTRIBUTING.md">
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll probably want to throw in rel="noopener noreferrer" here

@@ -61,6 +61,13 @@ def index(request):
}
return TemplateResponse(request, 'index.html', context)

def how_it_works(request):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@gitcoinbot gitcoinbot mentioned this pull request Jun 19, 2018
@thelostone-mc
Copy link
Member

thelostone-mc commented Jun 20, 2018

@mbeacom we need to think of better path for this than /funder/ & /contributor

@PixelantDesign I've attached the screenshots
UI looks alright from my end + mobile + tablet view == 👍

screencapture-0-0-0-0-8000-contributor-2018-06-20-21_20_05

screencapture-0-0-0-0-8000-funder-2018-06-20-21_12_53

Ps: @darkdarkdragon @PixelantDesign

Not a big fan of of the line going on top of the text.
One fix could be give white background to text and push it on top of the line using z-index

screen shot 2018-06-20 at 9 21 42 pm

Copy link
Contributor

@PixelantDesign PixelantDesign left a comment

Choose a reason for hiding this comment

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

In the original design the icons are closer to the numbers and the bottom section where it says "In some cases"...Can we shrink the graphics by 30%? They seem a little large.

can we also make the gray vertical line a few px thinner?

Looking great!

@darkdarkdragon
Copy link
Contributor Author

@thelostone-mc do you want it to look like this?:

image

@darkdarkdragon
Copy link
Contributor Author

@PixelantDesign I've made icons close to the numbers

@darkdarkdragon
Copy link
Contributor Author

screen shot 2018-06-21 at 18 59 31

@darkdarkdragon
Copy link
Contributor Author

screen shot 2018-06-21 at 18 58 36

@darkdarkdragon
Copy link
Contributor Author

screen shot 2018-06-21 at 19 00 55

@thelostone-mc
Copy link
Member

Ah it's fine the way it is ! Thanks @darkdarkdragon ^_^

@PixelantDesign
Copy link
Contributor

PixelantDesign commented Jun 22, 2018

@darkdarkdragon could you update the CTAs right above the footer (both pages)?

Developer Guide should link to this:
https://docs.google.com/document/d/1S8BLKJF7J5RbrfFw-mX0iYcy4VSc6-a1aQXtKT_ta0Y/edit?usp=sharing

Repo Maintainer Guide should link to this:
https://docs.google.com/document/d/1_U9IdDN8FIRMGAdLWCMl2BnqCTAv558QvyJiSWQfkbs/edit?usp=sharing

@darkdarkdragon
Copy link
Contributor Author

darkdarkdragon commented Jun 22, 2018

@PixelantDesign no problem, will do in half an hour

@darkdarkdragon
Copy link
Contributor Author

@PixelantDesign done

Added links to Developer's guide and Maintainer's guide
@ghost ghost assigned thelostone-mc Jun 23, 2018
@ghost ghost added the in progress label Jun 23, 2018
- reused existing css class
- fixed line overlappint content
- update URL -> /how/funder & /how/contributor
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.

LGTM !

@mbeacom I updated the url to keep it similiar to onboard (/onboard/contributor)

/funder -> /how/funder
/contributor -> /how/contributor

Copy link
Contributor

@SaptakS SaptakS left a comment

Choose a reason for hiding this comment

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

LGTM

@thelostone-mc thelostone-mc dismissed mbeacom’s stale review June 25, 2018 02:43

All comments have been addressed ^_^

@thelostone-mc thelostone-mc merged commit a8036b1 into gitcoinco:master Jun 25, 2018
@ghost ghost removed the in progress label Jun 25, 2018
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.

6 participants