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

fix of issue #76 Overhaul leaderboards page design #91

Merged
merged 6 commits into from
Dec 8, 2017

Conversation

ernaneluis
Copy link
Contributor

#76

Final layout in HTML/CSS

screenshot-2017-12-6 gitcoin push open source repos forward

@thelostone-mc
Copy link
Member

thelostone-mc commented Dec 6, 2017

@owocki I like the way this looks ^_^

@ernaneluis

  • what are your thought on extracting the css bit meant for the leaderboard, into an external file (lets say leaderboard.css rather than leaving it all within base.css ?

  • I get an list index out of range when my leaderboard is empty causing the page to throw an error.
    PS: @owocki thoughts on having a custom error page rather than spitting out the error on the UI?

Error:

web_1  |     return leaderboard(request, '')
web_1  |   File "/code/app/marketing/views.py", line 214, in leaderboard
web_1  |     amount_max = LeaderboardRank.objects.filter(active=True, leaderboard=key).values_list('amount').annotate(Max('amount')).order_by('-amount')[0][0]
web_1  |   File "/usr/local/lib/python2.7/site-packages/django/db/models/query.py", line 289, in __getitem__
web_1  |     return list(qs)[0]
web_1  | IndexError: list index out of range
web_1  | [05/Dec/2017 19:18:04] "GET /leaderboard/all_all HTTP/1.1" 500 76808

Screenshot(s)

with your changes:
screen shot 2017-12-06 at 7 49 51 am

In contrast to how it is currently is in master:

screen shot 2017-12-06 at 7 50 06 am

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

@thelostone-mc try loading this sql file into your database to get a real leaderboard http://bits.owocki.com/450U3e0U1h0C/marketing_leaderboardrank.sql

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

to @thelostone-mc 's point though, the code should probably properly handle a leaderboard with no content on it though. it could just say 'no activity found for this time range'

}

#leaderboard .winner-content .winner-position-third {
background-image: url('/static/v2/images/leaderboard-3.png');
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be filled out dynamically? check out bounty.local_avatar_url

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops i take this back. its the medal image

var max = parseInt($(this).attr('aria-valuemax'))
var now = parseInt($(this).attr('aria-valuenow'))
var width = (now * 100) / max
console.log(width)
Copy link
Contributor

Choose a reason for hiding this comment

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

errand console.log!

var width = (now * 100) / max
console.log(width)
$(this).css('width', width + '%')
// $(this).changeElementType('a'); // hack so that users can right click on the element
Copy link
Contributor

Choose a reason for hiding this comment

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

itd be nice if this was an anchor element so users could right click.

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

this looks really good. im excited!

background-image: url('/static/v2/images/leaderboard-3.png');
}


Copy link
Member

Choose a reason for hiding this comment

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

Can we do away with the extra empty lines ?
same in leaderboard.html

- make css of leaderboard into another css file, file leaderboard.css created
- removed empty lines
- make rows right clickable
@ernaneluis
Copy link
Contributor Author

suggestions were done :D

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

not sure what im doing wrong but this is what it looks like on my local http://bits.owocki.com/1C3g3W23383Z/Screen%20Shot%202017-12-06%20at%2012.27.44%20PM.png

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

tried incognito / with a cache refresh too..

@ernaneluis
Copy link
Contributor Author

ernaneluis commented Dec 6, 2017

I using this to get the avatar:

<img class="img-fluid" src="{{podium_items.local_avatar_url}}" >

and podium_items came from

items = LeaderboardRank.objects.filter(active=True, leaderboard=key).order_by('-amount')

the error is not been able to get the avatar:
screen shot 2017-12-06 at 20 40 33

what is the problem with this url http://localhost:8000/funding/avatar?repo=https://github.com/dylanjw&v=3 @owocki ?

if you access this url you going to get
screen shot 2017-12-06 at 20 42 22
any reason why this?

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

@ernaneluis yopu need to fill out the GITHUB_CLIENT_ID and other GITHUB_* keys in your local_settings.py

@owocki
Copy link
Contributor

owocki commented Dec 6, 2017

though this does not seem to be the issue with my local... theres something wacky about the layout on mine

@ernaneluis
Copy link
Contributor Author

ernaneluis commented Dec 6, 2017

@owocki can you show your console? to see if they are downlaoding the image?

@ernaneluis
Copy link
Contributor Author

@owocki can you have a look now. I made new commit.

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

😍 😍 😍 omg looks amazing

@@ -1,10 +1,17 @@
$(document).ready(function(){
$(".leaderboard_entry").each(function(){
$(this).changeElementType('a'); // hack so that users can right click on the element
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd still like for these items to be a tags because that means that a user can right click on them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is clickable but not using Anchor tag. It would need a lot of css fix in order to use the <a> tag. See comment bellow.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, ill make the change after merge

#leaderboard .leaderboard_entry .position{
color:lightgray;
}
/* #leaderboard .leaderboard_entry tr{
Copy link
Contributor

Choose a reason for hiding this comment

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

can u delete this dead code

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

anything we can do about the mobile view for the leaderboard?

http://bits.owocki.com/3H0E1F3f410v/Screen%20Shot%202017-12-06%20at%206.34.01%20PM.png

many of these issues can be fixed with just decreasing the font size

<div class="col body">
<div class="container-fluid" id="leaderboard">
{% include 'shared/bounty_def.html' %}
<div class="row mt-0 ml-1 pt-1 pl-1">
<select id="key">
Copy link
Contributor

Choose a reason for hiding this comment

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

i think well likely want to hide the 'key' field and just standardize on the monthly leaderboard for now. anyone disagree?

@@ -14,46 +15,98 @@
<div class="row no-gutter text-uppercase">
<div class="col-3" style="background: #eee; ">
{% include 'shared/current_balance.html' %}
{% include 'shared/sidebar_search.html' %}
<!-- {% include 'shared/sidebar_search.html' %} -->
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this module should still be on the page to keep parity with the other pages, no?

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 dont know, it will show a search side bar, which might be misleading to the user.

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

something on this PR broke the profile pages. not sure what it was -- maybe refactor'd CSS? http://bits.owocki.com/1V1k2E330y1M/Screen%20Shot%202017-12-06%20at%206.40.01%20PM.png

@owocki owocki closed this Dec 7, 2017
@owocki owocki reopened this Dec 7, 2017
@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

whoops accidental close

$('.clickable-row').click(function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here @owocki

now leaderboard css is only on the page leaderboard.html to avoid another pages conflict
@ernaneluis
Copy link
Contributor Author

@owocki I think I might fix the profile page as well please have a look.

@thelostone-mc
Copy link
Member

@ernaneluis sorry to be a pain but could we

  • do away with the extra white spaces like L30, L61
  • leave a space before { in css (this is quite a few places )
    #leaderboard .leaderboard_entry { instead of #leaderboard .leaderboard_entry{

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

@owocki I think I might fix the profile page as well please have a look.

to generate a profile locally just go to profile/xxx where xxx is anygithub username. the view is smart enough to hit the github API to get the profile information

@ernaneluis
Copy link
Contributor Author

but did you try it locally @owocki ?

@owocki
Copy link
Contributor

owocki commented Dec 7, 2017

@ernaneluis yes. for example, here is your profile generated locally http://bits.owocki.com/1W1X300Z3G0t/Screen%20Shot%202017-12-07%20at%209.27.21%20AM.png

@ernaneluis
Copy link
Contributor Author

@owocki i meant the new commits on the leaderboard page, please check if is correct now in order to merge

@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

looking now

@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

the leaderboard page looks great locally, but something on the branch has caused a regression in the profile pages, for example profile/xxx

vertical-align: middle;
margin-top: 20px;
}
.profile_details li:nth-child(2n+0) .dot,
Copy link
Contributor

Choose a reason for hiding this comment

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

-.profile_details li:nth-child(2n+0) .dot,

this should not be removed

}
.profile_details li:nth-child(2n+1) .dot,
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

its a small fix. easier for me to just fix than continue trying to talk about it here. paying out bounty and merging now

@owocki owocki merged commit 62af2e0 into gitcoinco:master Dec 8, 2017
@owocki
Copy link
Contributor

owocki commented Dec 8, 2017

https://gitcoin.co/leaderboard 🎉 @ernaneluis @thelostone-mc

@ernaneluis
Copy link
Contributor Author

@owocki I committed a small fix to solve this problem

screen shot 2017-12-08 at 21 30 17

#99

ethikz pushed a commit to ethikz/web that referenced this pull request Jan 24, 2018
owocki pushed a commit that referenced this pull request Apr 30, 2021
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.

3 participants