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

Use ImageOps.fit instead of Image.thumbnail #356

Merged
merged 1 commit into from
Feb 1, 2018

Conversation

poffdeluxe
Copy link
Contributor

Fixes: #355

Description

Now scales avatar photos by using ImageOps.fit instead of Image.thumbnail because Image.thumbnail will modify the image only if is larger than the given size.

Before:
avatar

After:
avatar 1

As this is my first PR contributing to this repo, please let me know if I missed anything or if I need to include more data here. I read the CONTRIBUTING.md guide but I'm nervous I missed something 😬

Checklist
Affected core subsystem(s)

dashboard/embed

Refers/Fixes

Fixes #355

@poffdeluxe poffdeluxe changed the title Use ImageOps.fit instead of Image.thumbnail to make sure avatars are … Use ImageOps.fit instead of Image.thumbnail Feb 1, 2018
@codecov
Copy link

codecov bot commented Feb 1, 2018

Codecov Report

Merging #356 into master will increase coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #356      +/-   ##
==========================================
+ Coverage   23.62%   23.62%   +<.01%     
==========================================
  Files          87       87              
  Lines        4123     4122       -1     
  Branches      492      492              
==========================================
  Hits          974      974              
+ Misses       3141     3140       -1     
  Partials        8        8
Impacted Files Coverage Δ
app/dashboard/embed.py 4.62% <50%> (+0.01%) ⬆️

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 37479c0...54d28b2. Read the comment docs.

@thelostone-mc
Copy link
Member

LGTM

Copy link
Contributor

@mbeacom mbeacom left a comment

Choose a reason for hiding this comment

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

lgtm :shipit: Thanks for the contribution!

Just a note, I made a change to local_avatar_url to ensure local and staging will reference the appropriate URL (versus always referencing https://gitcoin.co/) in b572fa6 - Tested this following the change. 💯

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