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

Maintain aspect ratio with height: auto; #5254

Merged
merged 1 commit into from Aug 17, 2016

Conversation

Projects
None yet
6 participants
@pmarsceill
Contributor

pmarsceill commented Aug 17, 2016

This PR fixes the Jekyll logo that was stretching when the browser was being resized:

Before (gross)

screenshot 2016-08-17 13 51 49

After (better)

screenshot 2016-08-17 13 55 10

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 17, 2016

Contributor

I see no difference at all in those images. And if I have to do a rapid
switch test to see it then the difference is irrelevant as this isn't outer
space. This is based on the images in my email.

On Wed, Aug 17, 2016, 12:55 PM Patrick Marsceill notifications@github.com
wrote:

This PR fixes the Jekyll logo that was stretching when the browser was
being resized:
Before (gross)

[image: screenshot 2016-08-17 13 51 49]
https://cloud.githubusercontent.com/assets/896475/17747216/c543340e-6481-11e6-841c-1f7a23befd54.png
After (better)

[image: screenshot 2016-08-17 13 51 49]

https://cloud.githubusercontent.com/assets/896475/17747304/3d2a5d62-6482-11e6-9743-a7568a4c8a40.png

You can view, comment on, or merge this pull request online at:

#5254
Commit Summary

  • Maintain aspect ratio with height: auto;

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#5254, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGFs6XY6IYYX-ezayX6IelCsffcL3clks5qg0sSgaJpZM4Jmt3K
.

Contributor

envygeeks commented Aug 17, 2016

I see no difference at all in those images. And if I have to do a rapid
switch test to see it then the difference is irrelevant as this isn't outer
space. This is based on the images in my email.

On Wed, Aug 17, 2016, 12:55 PM Patrick Marsceill notifications@github.com
wrote:

This PR fixes the Jekyll logo that was stretching when the browser was
being resized:
Before (gross)

[image: screenshot 2016-08-17 13 51 49]
https://cloud.githubusercontent.com/assets/896475/17747216/c543340e-6481-11e6-841c-1f7a23befd54.png
After (better)

[image: screenshot 2016-08-17 13 51 49]

https://cloud.githubusercontent.com/assets/896475/17747304/3d2a5d62-6482-11e6-9743-a7568a4c8a40.png

You can view, comment on, or merge this pull request online at:

#5254
Commit Summary

  • Maintain aspect ratio with height: auto;

File Changes

Patch Links:


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#5254, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAGFs6XY6IYYX-ezayX6IelCsffcL3clks5qg0sSgaJpZM4Jmt3K
.

@pmarsceill

This comment has been minimized.

Show comment
Hide comment
@pmarsceill

pmarsceill Aug 17, 2016

Contributor

I see no difference at all in those images

Here is a zoomed in view of the logo so it's easier to see on your phone <3:

Aspect ratio stretched (production):

screenshot 2016-08-17 15 44 03

Aspect ratio maintained (this branch):

screenshot 2016-08-17 15 45 47

Contributor

pmarsceill commented Aug 17, 2016

I see no difference at all in those images

Here is a zoomed in view of the logo so it's easier to see on your phone <3:

Aspect ratio stretched (production):

screenshot 2016-08-17 15 44 03

Aspect ratio maintained (this branch):

screenshot 2016-08-17 15 45 47

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Aug 17, 2016

Contributor

LGTM.

Thanks for this @pmarsceill! Nice catch. Looks like before on mobile the Jekyll logo would be stretched to about twice its height? If there's no downside to merging, I think it would add a nice level of polish to the site on mobile (and would make a great first contribution to Jekyll, hopefully first of many!) Thanks again @pmarsceill. 😄

Contributor

benbalter commented Aug 17, 2016

LGTM.

Thanks for this @pmarsceill! Nice catch. Looks like before on mobile the Jekyll logo would be stretched to about twice its height? If there's no downside to merging, I think it would add a nice level of polish to the site on mobile (and would make a great first contribution to Jekyll, hopefully first of many!) Thanks again @pmarsceill. 😄

@aaronshekey

This comment has been minimized.

Show comment
Hide comment
@aaronshekey

aaronshekey Aug 17, 2016

It's not very noticeable in a single screenshot delivered via email. That's fair. However, very few users of Jekyll's site will be consuming it via a single screenshot delivered by email. For the rest of the users out there using resizable browsers, we should be making sure the logo isn't horizontally compressed when it doesn't need to be, especially when the fix is a one-liner with no consequences.

aaronshekey commented Aug 17, 2016

It's not very noticeable in a single screenshot delivered via email. That's fair. However, very few users of Jekyll's site will be consuming it via a single screenshot delivered by email. For the rest of the users out there using resizable browsers, we should be making sure the logo isn't horizontally compressed when it doesn't need to be, especially when the fix is a one-liner with no consequences.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 17, 2016

Contributor

I'm still 👎, you are putting a bandaid on a single wound when there are 20 around it. If the image ends up expanding in any way because a browser is terrible at it's job, it's the headers fault for resizing itself and the fault of whoever designed the CSS, not the images fault which shouldn't need an explicit auto since modern browsers should implicitly set it...

Contributor

envygeeks commented Aug 17, 2016

I'm still 👎, you are putting a bandaid on a single wound when there are 20 around it. If the image ends up expanding in any way because a browser is terrible at it's job, it's the headers fault for resizing itself and the fault of whoever designed the CSS, not the images fault which shouldn't need an explicit auto since modern browsers should implicitly set it...

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 17, 2016

Contributor

You are comparing flexible UI's to a static UI and expecting the same tricks to stand up under scrutiny. To the on-looker sure it works, it solves the problem, but it doesn't solve the actual problem, which is the header is shifting in some way it shouldn't, that is the real problem that is causing this.

Contributor

envygeeks commented Aug 17, 2016

You are comparing flexible UI's to a static UI and expecting the same tricks to stand up under scrutiny. To the on-looker sure it works, it solves the problem, but it doesn't solve the actual problem, which is the header is shifting in some way it shouldn't, that is the real problem that is causing this.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 17, 2016

Contributor

I will concede though, I am unwiling to fix the problem myself, my backlog is already huge. So I guess I better either yeah you know the saying, it's LGTM.

Contributor

envygeeks commented Aug 17, 2016

I will concede though, I am unwiling to fix the problem myself, my backlog is already huge. So I guess I better either yeah you know the saying, it's LGTM.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Aug 17, 2016

Contributor

@jekyllbot: merge +site

Contributor

envygeeks commented Aug 17, 2016

@jekyllbot: merge +site

@jekyllbot jekyllbot merged commit 1b36e9f into jekyll:master Aug 17, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyll/lgtm Approved by @benbalter and @envygeeks.

jekyllbot added a commit that referenced this pull request Aug 17, 2016

@pmarsceill

This comment has been minimized.

Show comment
Hide comment
@pmarsceill

pmarsceill Aug 17, 2016

Contributor

I will concede though, I am unwiling to fix the problem myself, my backlog is already huge. So I guess I better either yeah you know the saying, it's LGTM.

Thanks. I'm More than willing to have a look at this.

Contributor

pmarsceill commented Aug 17, 2016

I will concede though, I am unwiling to fix the problem myself, my backlog is already huge. So I guess I better either yeah you know the saying, it's LGTM.

Thanks. I'm More than willing to have a look at this.

@pmarsceill pmarsceill deleted the pmarsceill:fix-logo-stretch branch Aug 17, 2016

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Sep 20, 2016

Contributor

This change should be submitted upstream and also add a comment that's it's a custom change.

Contributor

XhmikosR commented on 748b2f1 Sep 20, 2016

This change should be submitted upstream and also add a comment that's it's a custom change.

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