Skip to content

Conversation

@vlad2689
Copy link

@vlad2689 vlad2689 commented Jun 3, 2018

Description

WIP - Funder dashboard: #1177. Currently have done the front-end with all dummy data. Current state of things is this: https://imgur.com/a/YWirKIn

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

Frontend
Backend

Testing
Refers/Fixes

@owocki
Copy link
Contributor

owocki commented Jun 4, 2018

checking this out now

@ghost ghost assigned owocki Jun 4, 2018
@ghost ghost added the in progress label Jun 4, 2018
@@ -0,0 +1 @@
/* TODO: Move the inline styles from the html file here
Copy link
Contributor

Choose a reason for hiding this comment

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

yes pls

@owocki
Copy link
Contributor

owocki commented Jun 4, 2018

this is looking very good and is very exciting to me!

  1. just pushed up some merge conflict fixes
  2. some responsive design issues http://bits.owocki.com/1o3j2T1j3C3S/Screen%20Shot%202018-06-04%20at%205.37.06%20PM.png http://bits.owocki.com/3k2Q3z3F2V3s/Screen%20Shot%202018-06-04%20at%205.37.35%20PM.png
  3. rocketship img is missing http://bits.owocki.com/0s3f3R0g3P3A/Screen%20Shot%202018-06-04%20at%205.38.09%20PM.png
  4. 'View Etherscan' should be a link
  5. all of the data in the templates is hardcoded into the templates right now. could you make it so that it's passed down from the views.py file pls? this will allow the core team to hook up actual data
  6. WEEKLY MONTHLY YEARLY are not clickable

cc @mbeacom @thelostone-mc @SaptakS for their reviews too!


class Bounty(SuperModel):
"""Define the structure of a Bounty.
Copy link
Contributor

Choose a reason for hiding this comment

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

This new line is intentional and conforms to pep-257.

Can you re-add this new line?

{% trans "Reclaim Your Funds" %}
</div>
<div>
You have
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we wrap this and all of the other remaining relevant strings in trans throughout this file ?

@vlad2689
Copy link
Author

vlad2689 commented Jun 6, 2018

Thanks for the feedback @owocki, I'm very happy to be working on this as well. I'll be addressing the issues you pointed out. Definitely plenty left to fix and responsive design should have all the love it deserves. In terms of data being spitted out, I assume the goal is to have all the data passed down on GET and make updates with POSTs accordingly (when filtering)?

Just considering this, not sure if what I'm about to suggest is what we want, but - would it be acceptable to render the page with placeholders for data (basically empty divs) and use JS to render the data inside the elements after POST requests? Bear in mind, I am a bit biased towards doing things in JS even when it's not the best solution.

}

.funder-dashboard__header {
background-color: #3e00ff;}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we move the bracket to the next line
( You've already left a comment on shifting it to an external css file, so I'm good with that )

@thelostone-mc
Copy link
Contributor

@NedelescuVlad overall looking good :D

  • could we change to the font to use Muli and the fallback as sans-serif ?

  • remove the letter spacing for the numbers

screen shot 2018-06-07 at 3 01 40 pm

  • we could reuse the tags from the explorer for this

screen shot 2018-06-07 at 3 01 29 pm

It's there in dashboard.html
screen shot 2018-06-07 at 3 03 45 pm

  • Could you update the dropdown to use the existing styles
    checkout gitcoin/new you can reuse the code there

screen shot 2018-06-07 at 3 02 30 pm

  • Also this looks weird, the what's next to the avatar. (just an observation) :P

screen shot 2018-06-07 at 3 01 59 pm

I haven't checked out mobile / tablet view cause you haven't gotten to it. Let me know when I should ^_^

@mbeacom mbeacom changed the title Feature/vlad/funder dashboard WIP: Feature/vlad/funder dashboard Jun 7, 2018
@mbeacom mbeacom added the wip label Jun 7, 2018
@codecov
Copy link

codecov bot commented Jun 12, 2018

Codecov Report

Merging #1351 into master will decrease coverage by 0.36%.
The diff coverage is 12.92%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1351      +/-   ##
=========================================
- Coverage   27.77%   27.4%   -0.37%     
=========================================
  Files         140     141       +1     
  Lines       11359   11628     +269     
  Branches     1543    1595      +52     
=========================================
+ Hits         3155    3187      +32     
- Misses       8094    8331     +237     
  Partials      110     110
Impacted Files Coverage Δ
app/app/urls.py 86.04% <ø> (ø) ⬆️
app/dashboard/views.py 12.93% <10.2%> (-0.49%) ⬇️
app/dashboard/helpers.py 15.33% <12.17%> (-1.88%) ⬇️
app/economy/utils.py 84% <25%> (-11.24%) ⬇️
app/dashboard/models.py 50.27% <80%> (+0.11%) ⬆️
app/loader.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 ec39d3d...ef9fd02. Read the comment docs.

@vlad2689
Copy link
Author

vlad2689 commented Sep 13, 2018

Need to squash my commits, merge master into this again and fix the migration conflicts. Otherwise looks good imo.

One concern I have is regarding to @cache_page + @vary_on_cookie combination. Would it be possible for a user that has the same cookies as another to see that other user's funder dashboard? Would it ever be the case that 2 users have the same cookies, if they are logged in? (I noticed there's a sessionid cookie, is that set per user?)

Will do the merging and squashing when I wake up in about 10 hours

@vlad2689 vlad2689 force-pushed the feature/vlad/funder-dashboard branch 2 times, most recently from deeff94 to ff3ed6d Compare September 14, 2018 12:00
@vlad2689
Copy link
Author

vlad2689 commented Sep 14, 2018

@mbeacom Just finished squashing my commits. I ended up squashing some commits from other contributors to this PR as well... sorry for that but didn't know how to get around it.

You have my blessing to put this on staging =) ... (there's some stickler lint errors though.. not sure how I can see them as I can't seem to find them in the comments - might've marked some as resolved while they weren't)

@vlad2689 vlad2689 force-pushed the feature/vlad/funder-dashboard branch from ff3ed6d to d4d5d1c Compare September 14, 2018 12:13
@mbeacom
Copy link
Contributor

mbeacom commented Sep 14, 2018

@NedelescuVlad I hate to ask this... but can we maybe raise this in a separate branch or resolve the commit history overwrite? I don't want to muck up the commit history on master. Plus if you simply make a patch file from your branch, it'll squash your additional commits.

@vlad2689
Copy link
Author

@mbeacom Awesome, didn't know about patch files, that's exactly what i need. Will bring this to how it was and look into using a patch file instead

@vlad2689 vlad2689 force-pushed the feature/vlad/funder-dashboard branch from d4d5d1c to deeff94 Compare September 15, 2018 07:21
@vlad2689
Copy link
Author

vlad2689 commented Sep 15, 2018

@NedelescuVlad I hate to ask this... but can we maybe raise this in a separate branch or resolve the commit history overwrite? I don't want to muck up the commit history on master. Plus if you simply make a patch file from your branch, it'll squash your additional commits.

Could you have a look here please when you get the time? https://github.com/gitcoinco/web/compare/master...NedelescuVlad:feature/vlad/funder-dashboard-v3-throwaway?expand=1

I've squashed a bunch of commits into that "Add funder dashboard" commit but am unsure what to do with the ones that belong to other contributors. Am I fine to also squash those since they aren't on master at the moment anyway? Otherwise I'll need to be re-applying commits with patches in order to get the history in a state where I can squash only my commits further.

I'm thinking that when we're happy with how the history looks on that branch I'll replace this one's history with that one's.

IMO I would squash them all and merge this feature as 1 commit since the commits from other people are few. Unless I'm missing something on how this can mess up with the master history... which I can't obviously see as none of the commits on this branch are merged yet.

@PixelantDesign
Copy link
Contributor

Hey @NedelescuVlad Thanks for all of the hard work! We'd like to take this back in and push the V1 out. Please click submit work so that you can get paid!

@vlad2689
Copy link
Author

@PixelantDesign thank you very much and sorry I couldn't get this further :) Good luck with tidying up

@PixelantDesign
Copy link
Contributor

Hey @NedelescuVlad It looks like some of the data points are displaying on the dashboard, are you able to help take a quick look?

@vlad2689
Copy link
Author

vlad2689 commented Sep 18, 2018 via email

@vlad2689
Copy link
Author

@thelostone-mc could we close this PR and leave a link to the new PR this got moved to?

@thelostone-mc
Copy link
Contributor

@NedelescuVlad I'm okay with that. Moving this over to #2239

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.

10 participants