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.

Sign up for GitHub

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

Jump to bottom

Add showcase section to hackathons #7011

Merged
merged 11 commits into from
Jul 9, 2020
Merged

Add showcase section to hackathons #7011

merged 11 commits into from
Jul 9, 2020

Conversation

zoek1
Copy link
Contributor

@zoek1 zoek1 commented Jun 29, 2020 •
edited
Loading

Description
Refers/Fixes

#6769

Testing

image
image
image

PixelantDesign
PixelantDesign approved these changes Jun 29, 2020
View reviewed changes
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.

Looks great!

@PixelantDesign
Copy link
Contributor

What do you think @fluffays @willsputra

danlipert
danlipert approved these changes Jun 29, 2020
View reviewed changes
Copy link
Contributor

@danlipert danlipert left a comment

Choose a reason for hiding this comment

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

really nice

@fluffays
Copy link

fluffays commented Jun 29, 2020 •
edited
Loading

Looks good 🚀 ! I've got 3 small suggestions:

  1. Hacking spotlights" is a section to showcase cool bounties from the hackathon. Under the description of each bounty, I would also add the winning hackers (where applicable). Ideal format would be:
    Winners:
    Prize # 1 - hacker_name
    Prize # 2 - hacker_profile
    etc.

  2. "Top <hackathon_name> Hackers" => I think it's a bit redundant to include the hackathon name in the title here (since this section is displayed in the hackathon's navigation. I suggest changing this section's title to "Hackers' Wall of Fame".

  3. "Top <hackathon_name> Hackers" => could we try a vertical alignment of hacker_avatar + hacker_name, instead of putting both on a horizontal line? Something like this:

Screenshot 2020-06-29 at 16 37 51

This might also help with layout on mobile.

@PixelantDesign
Copy link
Contributor

What do you think @vs77bb @connoroday since y'all will be using this page?

@owocki
Copy link
Contributor

owocki commented Jun 30, 2020

llooks pretty exciting. hoping to get this in for SFBW

octavioamu
octavioamu reviewed Jul 2, 2020
View reviewed changes
app/assets/v2/css/gitcoin.css Outdated
@@ -71,6 +71,7 @@ h4,
background-position: 0px 0px;
background-repeat: no-repeat;
position: relative;
z-index: 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this needed?

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 used over the site in some places the next box need to overlap the header, for example on onboard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, this change was from another PR isn't required here.

octavioamu
octavioamu suggested changes Jul 2, 2020
View reviewed changes
Copy link
Contributor

@octavioamu octavioamu left a comment

Choose a reason for hiding this comment

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

left a comment about z-index.
Also can we apply the default input styles we use in other parts of the site and align them horizontally ?

@zoek1 zoek1 requested a review from octavioamu July 3, 2020 06:12
@PixelantDesign
Copy link
Contributor

Excited for this one!

@PixelantDesign
Copy link
Contributor

@frankchen07 need 1 more review!

frankchen07
frankchen07 approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@frankchen07 frankchen07 left a comment

Choose a reason for hiding this comment

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

LGTM

some quick comments:

anything with a dropdown looks to be editable, where as things like bounties and coders we're pulling directly?

add spotlight = add sponsor & description?

@codecov
Copy link

codecov bot commented Jul 9, 2020 •
edited
Loading

Codecov Report

Merging #7011 into master will increase coverage by 0.02%.
The diff coverage is 43.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7011      +/-   ##
==========================================
+ Coverage   26.27%   26.30%   +0.02%     
==========================================
  Files         300      300              
  Lines       29343    29359      +16     
  Branches     4319     4321       +2     
==========================================
+ Hits         7711     7724      +13     
- Misses      21361    21364       +3     
  Partials      271      271
Impacted Files Coverage Δ
app/app/urls.py 86.20% <ø> (ø)
app/dashboard/views.py 10.57% <12.50%> (+<0.01%) ⬆️
app/dashboard/router.py 39.35% <71.42%> (+0.66%) ⬆️
app/dashboard/models.py 48.54% <100.00%> (+0.01%) ⬆️
app/dashboard/embed.py 31.60% <0.00%> (+3.44%) ⬆️

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 e612086...475c489. Read the comment docs.

@danlipert danlipert merged commit e563a7d into gitcoinco:master Jul 9, 2020
octavioamu
octavioamu reviewed Jul 10, 2020
View reviewed changes
app/dashboard/router.py
Comment on lines +69 to +70
def get_winners(self, obj):
return reduce(lambda total, prize: total + len(prize.paid), Bounty.objects.filter(event=obj).distinct(), 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

seems even with one winner project still getting 0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@octavioamu octavioamu octavioamu approved these changes

@frankchen07 frankchen07 frankchen07 approved these changes

@thelostone-mc thelostone-mc thelostone-mc approved these changes

@danlipert danlipert danlipert approved these changes

@PixelantDesign PixelantDesign PixelantDesign approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zoek1 @PixelantDesign @fluffays @owocki @danlipert @octavioamu @thelostone-mc @frankchen07