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

Sizes being computed from height #43

Closed
crstauf opened this issue Jan 21, 2015 · 11 comments
Closed

Sizes being computed from height #43

crstauf opened this issue Jan 21, 2015 · 11 comments

Comments

@crstauf
Copy link

crstauf commented Jan 21, 2015

Is height a variable in the computation of the sizes attribute? I've got a slider where lazysizes is setting the sizes attribute at the height of the slider, rather than it's width, which returns a stretched low-resolution image... is this proper behavior?

@aFarkas
Copy link
Owner

aFarkas commented Jan 21, 2015

No, this is absolutely not the intended behavior. It's either a bug or you something might be wrong with your CSS or the slider.

Is this bug also happening if you do add the following CSS to your image:

.slider img {
    display: block;
    width: 100%;
}

From the documentation. The sizes is computed by taking the width of the image if it is over 50px this is used otherwise the script is going up the tree until it finds a parent, with a width over 50. In case your image has a width of "auto" things can go wrong badly.

Does this help?

In case not, I would need a testcase, a link to a demo or maybe some more information / digging by you. (you can also mail me).

In case you want to add a breakpoint to see how this is computed, here is the code calculating the width.

@crstauf
Copy link
Author

crstauf commented Jan 21, 2015

currently having the issue on a dev site, and we just changed the code, but I'll attempt to recreate the issue later and provide it asap.

@aFarkas
Copy link
Owner

aFarkas commented Jan 27, 2015

@crstauf
I really would like to fix this ;-).

@crstauf
Copy link
Author

crstauf commented Jan 28, 2015

absolutely; will get a recreation to you asap.

@crstauf
Copy link
Author

crstauf commented Jan 28, 2015

okay, here's a CodePen: http://codepen.io/anon/pen/jELqrb
replicates the situation where I experience (what i think is) lazysizes calculating sizes by height rather than width.

@aFarkas
Copy link
Owner

aFarkas commented Jan 28, 2015

@crstauf
Thanks for the testcase. The lazySizes calculation works here as designed, so it isn't really a bug. If you remove the data-src and the data-srcset you will, that the image with the 1px image will have a width equal to the calculated sizes property.

For your usecase there need to be done multiple things to get it right. What your actually want is something like object-fit: cover; (see: https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit). For this not only the width but also the height needs to be considered, before selecting an image. This is something the standard does currently not support and therefore lazySizes also does not support.

But I have some semi-good news for you. I can make your usecase work as soon as you are using the lqip pattern. In case you haven't used this pattern because you want to save bandwidth, you can use an extreme small image. The only thing lazySizes needs to get the calculation right is an image, which has the same or "similiar" aspect ratio. So you could use for example an image width a width of 29 and a height of 10 as you image.

@aFarkas
Copy link
Owner

aFarkas commented Jan 28, 2015

Just thought a little bit more and there are also other ways to workaround this problem. The best way is to already initially maintain the aspect ratio using the intrinsic ratio CSS technique.

Due to the fact that I see some width/height attributes the aspect ratio is known by the backend, so it should be simple to implement it. The best thing of course is to do this in the backend, but of course you can also use JS to do this. By simply wrapping the image element in a container with height: 0 and calculating it's padding-bottom or padding-top by deviding the (img[height] / img[width] * 100) +'%'.

I hope this helps? If you want/need I can create a fiddle.

@crstauf
Copy link
Author

crstauf commented Jan 28, 2015

Hey Alexander, thanks for the feedback and tips. My curiosity is why the
calculation generates a sizes attribute of ~400px, and not the width, 320px?

On Wednesday, January 28, 2015, Alexander Farkas notifications@github.com
wrote:

Just thought a little bit more and there are also other ways to workaround
this problem. The best way is to already initially maintain the aspect
ratio using the intrinsic ratio CSS technique
https://github.com/aFarkas/lazysizes#specifying-image-dimensions-minimizing-reflows-and-avoiding-page-jumps.

Due to the fact that I see some width/height attributes the aspect ratio
is known by the backend, so it should be simple to implement it. The best
thing of course is to do this in the backend, but of course you can also
use JS to do this. By simply wrapping the image element in a container with height:
0 and calculating it's padding-bottom or padding-top by deviding the (img[height]
/ img[width] * 100) +'%'.

I hope this helps? If you want/need I can create a fiddle.


Reply to this email directly or view it on GitHub
#43 (comment).

Caleb Stauffer

@aFarkas
Copy link
Owner

aFarkas commented Jan 28, 2015

The width is 320px and the height is 341px + 60px (= 401px) and you are saying that image should have min-width/min-height of 100% and due to the fact, that the transparent image is 1x1, this images becomes 401x401 to be at least the height, while maintaining aspect ratio). This is also the fact, why using a 29x10 (or a 3x1) image placeholder would work, because than the image would become 1160x401.

@crstauf
Copy link
Author

crstauf commented Jan 28, 2015

Because lazySizes looks at the image and it's container, and uses whichever
is larger! Oh! Thanks for the help/explanation! :-)

On Wednesday, January 28, 2015, Alexander Farkas notifications@github.com
wrote:

The width is 320px and the height is 341px + 60px (= 401px) and you are
saying that image should have min-width/min-height of 100% and due to the
fact, that the transparent image is 1x1, this images becomes 401x401. This
is also the fact, why using a 29x10 image placeholder would work, because
than the image would become 1160x401.


Reply to this email directly or view it on GitHub
#43 (comment).

Caleb Stauffer

@aFarkas
Copy link
Owner

aFarkas commented Jan 28, 2015

Although, not 100% satisfying, I'm closing this issue. I think we it shows some workarounds + it indeed also improved the calculation (see code changes above). Hope this is fine.

@aFarkas aFarkas closed this as completed Jan 28, 2015
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

No branches or pull requests

2 participants