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

Visual Improvements for Results page #4167

Merged

Conversation

MaxStalker
Copy link
Contributor

Description

Elements on results page had inconsistent design (font sizes, margins, padding, etc.) and in a lot of cases were using inline styles instead of classes.

Checklist
  • Read and conforms to the contributor guidelines
  • Read and conforms to the style guidelines
  • Tests pass and code coverage has not decreased
  • Changes don't break existing behavior
  • Commit message follows commit guidelines
  • Completed: Code review by core team and any requested changes
Affected core subsystem(s)

UI was slightly adjusted, but it's more of a styling changes - no new functionality

Refers/Fixes
Testing and Sign-off

I've launched local copy of web repository using docker as described in this article: https://docs.gitcoin.co/mk_setup/
Visually most of the blocks were working. Some were missing, probably because there were no data in local database. Though I've tested layout with static html (which was later removed) similar to the one on live site and it was working.

Contributor
  • Read and followed the Contributor Guidelines
  • Tested all changes locally
  • Verified existing functionality
  • Ran make test and everything passed!
Reviewer
  • Affirm contributor guidelines have been followed and requested changes made
  • CI tests and linting pass
  • No conflicts (migrations, files, etc)
  • Regression tested against staging or local deployment
Funder
  • Validated requested changes were made to specification
  • Bounty payout released to the contributor

@codecov
Copy link

codecov bot commented Apr 10, 2019

Codecov Report

Merging #4167 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4167      +/-   ##
==========================================
- Coverage   30.22%   30.18%   -0.04%     
==========================================
  Files         209      209              
  Lines       16625    16625              
  Branches     2223     2223              
==========================================
- Hits         5025     5019       -6     
- Misses      11419    11425       +6     
  Partials      181      181
Impacted Files Coverage Δ
app/dashboard/embed.py 28.16% <0%> (-3.45%) ⬇️

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 d8282aa...48f5352. Read the comment docs.

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.

left a few comments.
could you also add screenshots/ recordings of the updated page on desktop + tablet + mobile (aka responsive )

LGTM otherwise ^_^

app/assets/v2/css/base.css Outdated Show resolved Hide resolved
app/assets/v2/css/lib/typography.css Outdated Show resolved Hide resolved
app/assets/v2/css/lib/typography.css Outdated Show resolved Hide resolved
app/assets/v2/css/lib/typography.css Outdated Show resolved Hide resolved
app/assets/v2/css/lib/typography.css Outdated Show resolved Hide resolved
app/assets/v2/css/results.css Outdated Show resolved Hide resolved
app/assets/v2/css/results.css Outdated Show resolved Hide resolved
app/assets/v2/css/results.css Outdated Show resolved Hide resolved
app/retail/templates/results.html Outdated Show resolved Hide resolved
app/retail/templates/results.html Show resolved Hide resolved
@owocki
Copy link
Contributor

owocki commented Apr 10, 2019

anyone got a screenshot of what this looks like? or can we put it up on stage?

@MaxStalker
Copy link
Contributor Author

@thelostone-mc Got it, will update that CSS and prepare images/video capture
@owocki I will provide it together with updates mentioned by Aditya.

@MaxStalker
Copy link
Contributor Author

Here you can checkout video of responsiveness:
https://www.loom.com/share/4838dd879922492c891263389849bb3f

@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 12, 2019

^ the font-size comments need to be addressed but other than that looks solid 🙌
could you rebase + conflicts

@MaxStalker MaxStalker force-pushed the design/results-visual-improvement branch from 3754a3b to d5de5ce Compare April 12, 2019 10:19
@MaxStalker
Copy link
Contributor Author

@thelostone-mc done and done :)

@thelostone-mc
Copy link
Member

Ah this is a lil embarrasing ! mind resolving the conflcits one last time ?
we can get this in after that

@thelostone-mc
Copy link
Member

@MaxStalker ^ Any chance we can get this wrapped up in the next few days ?

@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 24, 2019

@MaxStalker Ah i think something got messy during the merge with master! we've got 228 commits in this PR now :P

@owocki could use help some help testing this. Checked out only code 😅

@thelostone-mc thelostone-mc requested a review from owocki April 24, 2019 16:51
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