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

Quickstart screen for bounty creation flow #1029

Merged
merged 19 commits into from
May 29, 2018

Conversation

SaptakS
Copy link
Contributor

@SaptakS SaptakS commented Apr 30, 2018

Description
  • Frontend for the quickstart screen has been added.
  • Functionalities like video lightbox and don't show again.
  • Integrating with the bounty creation flow.
Checklist
  • linter status: 100% pass
  • changes don't break existing behavior
  • commit message follows commit guidelines
Affected core subsystem(s)
Testing
Refers/Fixes

Fixes: #982
Refs: #429

@SaptakS
Copy link
Contributor Author

SaptakS commented Apr 30, 2018

Following is the screenshot as it looks in Chrome in MacOS. Some work still needs to be done in responsiveness.
screencapture-127-0-0-1-8000-bounty-quickstart-2018-04-30-23_56_29

@mbeacom mbeacom added frontend This needs frontend expertise. in progress bounties labels Apr 30, 2018
@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 30, 2018

@SaptakS Oh you might wanna use 2 spaces as against 4 in .css files :P

The box svg + green background for the issue svgs still feels a lil off 😓

@SaptakS
Copy link
Contributor Author

SaptakS commented May 1, 2018

Oh you might wanna use 2 spaces as against 4 in .css files :P

Yes yes. I haven't checked the linting. I will definitely do that. No worries.

@SaptakS
Copy link
Contributor Author

SaptakS commented May 1, 2018

The box svg + green background for the issue svgs still feels a lil off

I thought it goes well with the current design of the entire website. What would you suggest?

@thelostone-mc
Copy link
Member

I figured we'd have the svgs in green against a white background but yeah that might throw off the color balance !! Let's stick to the design for now in that case

( Oh btw how does it look when you swap out the grey with light shade from the filters on the explorer pages ? )

@codecov
Copy link

codecov bot commented May 4, 2018

Codecov Report

Merging #1029 into master will increase coverage by 0.15%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1029      +/-   ##
==========================================
+ Coverage   31.08%   31.24%   +0.15%     
==========================================
  Files         122      120       -2     
  Lines        8521     8404     -117     
  Branches     1117     1100      -17     
==========================================
- Hits         2649     2626      -23     
+ Misses       5764     5669      -95     
- Partials      108      109       +1
Impacted Files Coverage Δ
app/app/urls.py 94.28% <ø> (ø) ⬆️
app/dashboard/views.py 16.89% <57.14%> (+0.56%) ⬆️
app/dashboard/admin.py 72.41% <0%> (-4.4%) ⬇️
app/marketing/admin.py 76% <0%> (-4.33%) ⬇️
app/app/settings.py 83.61% <0%> (-1.7%) ⬇️
app/faucet/admin.py 64.51% <0%> (-1.11%) ⬇️
app/marketing/models.py 65.98% <0%> (-0.46%) ⬇️
app/enssubdomain/views.py 22.9% <0%> (-0.41%) ⬇️
app/dashboard/notifications.py 16.74% <0%> (-0.31%) ⬇️
app/retail/views.py 39.09% <0%> (ø) ⬆️
... and 12 more

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 efd66b3...86bc240. Read the comment docs.

@SaptakS SaptakS force-pushed the feature/quickstart-screen branch from ce35d93 to ce710e6 Compare May 8, 2018 06:19
@@ -719,6 +719,17 @@ def bounty_details(request, ghuser='', ghrepo='', ghissue=0, stdbounties_id=None
return TemplateResponse(request, 'bounty_details.html', params)



def quickstart(request):

Choose a reason for hiding this comment

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

E303 too many blank lines (3)

@SaptakS SaptakS force-pushed the feature/quickstart-screen branch 2 times, most recently from c4414e1 to 6804829 Compare May 8, 2018 09:45
@SaptakS SaptakS force-pushed the feature/quickstart-screen branch from 6804829 to 7c2dd8b Compare May 9, 2018 07:41
@SaptakS SaptakS force-pushed the feature/quickstart-screen branch from dd60f0e to 42beea2 Compare May 10, 2018 09:50
@thelostone-mc
Copy link
Member

thelostone-mc commented May 10, 2018

@SaptakS so a good chunk of the code base uses the following classes for font-size to ensure we don't have different font-size!

I know the design had the font-size bigger, but should we update it to match what is being used ?
Checkout out "localhost:8000/onboard" [ Comment out this line ]


Part 1

screen shot 2018-05-10 at 3 28 48 pm

- Use a mix of : `font-subheader` + `font-title` + `font-body` for this : - Checkbox , we should reuse the styles like [here](https://github.com/gitcoinco/web/blob/master/app/dashboard/templates/shared/sidebar_search.html#L104-L110)
Part 2

screen shot 2018-05-10 at 3 34 29 pm

  • Use a mix of : font-subheader + font-title + font-caption
  • Also can we use font-awesome 5 circle + cross instead of the image? That looks nicer :P

Part 3

screen shot 2018-05-10 at 3 36 26 pm

The Metamask status is longer as compared to the status present on the dashboard & bounty details page. We could annoy @PixelantDesign to make it uniform :P


Part 4

screen shot 2018-05-10 at 3 37 41 pm

  • This could use margin-top :P
  • Oh we should change the title to use "Muli" (and set the line-height : 0)

Part 5

screen shot 2018-05-10 at 3 41 54 pm

  • We should make the checkboxes uniform ! Both are different from what's used in the explorer
    Source code is here Copy + Paste \m/
  • Also thoughts on making the Fund Issue smaller ? (I know it's like this in the design but we don't have super wide buttons anywhere :P )

Part 6

When I click on fund issue without filling the issue details and stuff, it redirects me back to quickstart page ? 😅

x


Part 7 - Mobile

x1

  • The font seems to big but that would be fixed when we reuse what we have
    ( as mentioned it Part1 :P )
  • @PixelantDesign I'm not a fan of the grey :P
  • Oh could we remove the padding-bottom on mobile ?

@SaptakS Feel free to buy me food for helping with the review :P
I like bacon

Also @mbeacom would it be okay to merge this in once these issues are addressed & disable the url so that @SaptakS doesn't spend his life resolving conflicts ?

@mbeacom
Copy link
Contributor

mbeacom commented May 16, 2018

@SaptakS Would you mind resolving the conflicts on this PR? Once that's done, we'll get this pushed to staging.

@SaptakS
Copy link
Contributor Author

SaptakS commented May 23, 2018

"Create a Github Issue" link doesnt go anywhere

What should I hyperlink this to? @owocki @PixelantDesign

@SaptakS
Copy link
Contributor Author

SaptakS commented May 23, 2018

how do i use the ' Don't show again' link?

Right now what I have is you need to click the checkbox and then click "Ok, I'm ready" button. And the localstorage value gets set so that next time you visit a new funder issue, it doesn't redirect.

I am still not sure whether to do it via JS or do we maintain a table to just save the information about users who have clicked on Don't show so that we can redirect from the backend instead. @owocki @mbeacom @thelostone-mc

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

I am committing the changes I've referenced in my comments. This just needs tested on staging. I'll deploy it once I commit my changes.

</div>
<div class="col-lg-2 col-sm-12">
<p>
<a class="btn btn-sm btn-info mt-3 pulseClick" role="button" id="submit" href="/faucet" target="_blank" rel="noopener noreferrer">{% trans "View Faucet" %}</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is merely a note since it was already like this, but we'll want to be mindful of replacing hard coded URLs in templates with {% url 'url_reverse_name' %}


context = {
'active': 'video',
'title': 'Quickstart Video',
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 want to wrap this in gettext_lazy via: _()

@ratelimit(key='ip', rate='5/m', method=ratelimit.UNSAFE, block=True)
def get_quickstart_video(request):
"""Show quickstart video."""

Copy link
Contributor

Choose a reason for hiding this comment

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

No new line between method start/docstring and the first line of logic.

@@ -710,6 +710,16 @@ def bounty_details(request, ghuser='', ghrepo='', ghissue=0, stdbounties_id=None
return TemplateResponse(request, 'bounty_details.html', params)


def quickstart(request):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm adding staff_member_required to the new views until we're ready to launch quickstart

<li>{% trans "4) Confirm the transaction in metamask" %}</li>
</ol>
<p>
{% trans "Once confirmed on blockchain, your bounty is posted to Gitcoin!
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to be careful using {% trans "" %} spanning multiple lines with heavy whitespace between line segments.

@@ -0,0 +1,19 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs squashed. You can do that via make compress-images or simply svgo app/assets/v2/images/Lightbulb.svg for the time being.

@ghost ghost assigned mbeacom May 23, 2018
@mbeacom
Copy link
Contributor

mbeacom commented May 23, 2018

@SaptakS As we discussed previously, I think storing the preference in the DB is a solid choice, so we can ensure persistence regardless of where the user is browsing from.

If we wanted to offload the check for this and avoid the query in most cases, we could store it in the DB once they opt in to Don't Show Again and local storage, only querying for that preference data if the local storage key doesn't exist. Anyone have strong opinions on this?

@PixelantDesign
Copy link
Contributor

Looking good on the quick start!

  • Please update text: More funding tips --> See Developer Guide
  • Set tracking to zero on headers
  • Take body copy to the next size

Funder Form

  • Gas section is missing! Yikes
  • add some space above creator details

Thanks!

@ghost ghost assigned SaptakS May 28, 2018
@SaptakS
Copy link
Contributor Author

SaptakS commented May 28, 2018

@PixelantDesign Done!

Wait for developers across the world to discover your bounty and
start working on your issue.
{% endblocktrans %}
{% trans "Once confirmed on blockchain, your bounty is posted to Gitcoin! Wait for developers across the world to discover your bounty and start working on your issue." %}
Copy link
Member

Choose a reason for hiding this comment

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

@SaptakS Wanna break this up into multiple lines? Avoids horizontal scrolling 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had this broken but then it was causing problem with {% trans. Instead of rendering, it was showing {% trans as normal text.

@mbeacom mbeacom changed the title WIP: Quickstart screen for bounty creation flow Quickstart screen for bounty creation flow May 29, 2018
mbeacom
mbeacom previously approved these changes May 29, 2018
app/app/urls.py Outdated
@@ -73,6 +73,7 @@
url(r'^explorer/?', dashboard.views.dashboard, name='explorer'),

# action URLs
url(r'^bounty/quickstart/?', dashboard.views.quickstart, name='quickstart'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving forward, can we use re_path and path for new urls?

Copy link
Contributor

@mbeacom mbeacom 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 mbeacom merged commit 22e5b36 into gitcoinco:master May 29, 2018
@ghost ghost removed the needs review label May 29, 2018
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.

looked good to me

mbeacom added a commit that referenced this pull request May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bounties frontend This needs frontend expertise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants