search

vvo / in-viewport

Get a callback when any element becomes visible in a viewport (body or custom viewport)
MIT License
276 stars 24 forks source link

It doesn't show up as visible if height is 0 #16

Open cusspvz opened 8 years ago

cusspvz commented 8 years ago

It should show an element as visible independently of its height, but instead, it doesn't.

PoC

captura de ecra 2016-03-18 as 14 57 36 captura de ecra 2016-03-18 as 14 57 20
vvo commented 8 years ago

Hi, thanks for the bug report!

Can you create a new failing test and submit a PR with a fix if needed? Everything needed is available in the repo to do so.

Let me know.

cedricdelpoux commented 7 years ago

Thank you for reporting this bug. I was looking for what was happening :D My solution was to set min-height: 1px;. But I'm not very happy with this solution

tzi commented 7 years ago

Yes if an element has no height, it is not visible. This behavior is intentional and ISO with the $(':hidden') selector of jQuery

Here is the line that is responsible of this feature : https://github.com/vvo/in-viewport/blob/master/in-viewport.js#L143

is it a problem? In which use case?

Cheers, Thomas.

cedricdelpoux commented 7 years ago

For exemple, I have a list of blog post. I need to load social shares (facebook count, Google plus count...) on each post. So I want to load my components when blog post is in the viewport. Before loaded, wrapper are empty so height is 0

tzi commented 7 years ago

Thanks for the details!

This lib is made to detect "when an element becomes visible in a viewport". Perhaps we should simplify and detect only when an element is in a viewport, because this lib is uses for lazy loading most of the time.

It would be simpler et prevent some issues with developers. Do you agree @vvo?

:warning: This would mean to create a major version.

vvo commented 7 years ago

I am not sure to get the distinction and your proposal. For sure I don't mind doing breaking changes but can you detail a bit more what do you mean?

niksy commented 7 years ago

As @tzi mentioned, it could prevent common development issues since elements which are tested for the viewport presence are usually ones which should be lazy loaded and their placeholders usually don’t have any content, therefore, height with 0px.

Maybe this could also be an option? Or just cater to that common use case by default without any option?

cusspvz commented 7 years ago

@vvo what if the detection of 0px heighted elements was implementad as a method option?

TehShrike commented 7 years ago

This prompted me to copy the visibility function, remove some of the functions I didn't need, and comment out these lines: https://github.com/vvo/in-viewport/blob/04b22a680b104bc41b2f1f260083e114d0e81721/in-viewport.js#L153-L155

Works dandily.

cusspvz commented 7 years ago

I agree with @TehShrike solution, it would return a0 instead of a false, and wouldn't break most of use cases (except typed-check based).

vvo commented 7 years ago

I am down for changing the behavior of the library, send a PR and we will discuss it!

TehShrike commented 7 years ago

I think the confusion came from the fact that the library is named "in-viewport", but it internally uses a function named "isVisible". Those two things aren't always the same.

rulrok commented 6 years ago

I would appreciate if this behaviour could be passed in an options parameter.