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

Issue #4204 - Homepage video preview blocks #4205

Merged
merged 12 commits into from
Apr 16, 2019

Conversation

MaxStalker
Copy link
Contributor

Description

Video previews on home page are using SVGs at the moment. Text inside those is not outlined so it renders with a default serif font and is not centered in that block.
Here's how it looks currently:
current-video-blocks

And here's proposed solution:
new-video-blocks

Checklist
Affected core subsystem(s)

User interface

Refers/Fixes

This PR fixes #4204

Testing and Sign-off

Tested on local version of site using docker. Checked with different device screen width to ensure responsiveness:
https://www.loom.com/share/594acd535ed242de8d61544eddfac5e9

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

@MaxStalker MaxStalker changed the title Design - Homepage video preview blocks Issue 4204 - Homepage video preview blocks Apr 16, 2019
@MaxStalker MaxStalker changed the title Issue 4204 - Homepage video preview blocks Issue #4204 - Homepage video preview blocks Apr 16, 2019
@codecov
Copy link

codecov bot commented Apr 16, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4205      +/-   ##
==========================================
- Coverage   30.37%   30.33%   -0.04%     
==========================================
  Files         206      206              
  Lines       16404    16404              
  Branches     2156     2156              
==========================================
- Hits         4982     4976       -6     
- Misses      11242    11248       +6     
  Partials      180      180
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 e07c199...1c7f66b. Read the comment docs.

thelostone-mc
thelostone-mc previously approved these changes Apr 16, 2019
thelostone-mc
thelostone-mc previously approved these changes Apr 16, 2019
@@ -275,7 +264,7 @@ <h2>{% trans "Get to know us better" %}</h2>
</div>
</div>
</div>
<div id="articles">
<div id="articles" class="articles">
Copy link
Member

Choose a reason for hiding this comment

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

@MaxStalker hmm could we keep it as an id ?
Unless you see a reason where this would be reused again elsewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this particular case ids were used for styling - which is "no-no", since it creates more problems in the future. I've left ids there in case there are links elsewhere that reference them.
We can refactor "articles" to "home_articles" (or something similar) if you think this might introduce name collisions later.

@thelostone-mc
Copy link
Member

@MaxStalker could you address my comments ? Otherwise LGTM

@MaxStalker
Copy link
Contributor Author

@thelostone-mc I've updated the code. Can you give it another look?

@thelostone-mc thelostone-mc merged commit 977ad5d into gitcoinco:master Apr 16, 2019
@thelostone-mc
Copy link
Member

thelostone-mc commented Apr 16, 2019

thanks @MaxStalker ^^ and yup it looks good ^^

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.

Design - Video preview blocks on Home page
2 participants