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

If element height is 0, return it instead of `false` #30

Open cusspvz opened 7 years ago

cusspvz commented 7 years ago

Change proposed by @TehShrike on #16.

This changes the behavior on cases where the height of the element is 0.

Instead of returning false, it will return 0 instead.

This happens because the element is on screen but, as it has a height of 0, it is not displayed. The result is afalsy one as before, but now people can check if it is indeed on screen by checking by type (boolean or number)

vvo commented 7 years ago

If I try to summarise it (sorry I am not actively using my own lib).

The current behavior (before this PR) is to consider element not in the viewport when their height and width is 0.

With your change, they will now be considered in the viewport as soon as they are in the viewport, no matter their height.

If I get this right, I am down for the change and we can move forward, let me know

tzi commented 7 years ago

For me, you summarized well the problem.

:+1: to merge, and to release a major version

vvo commented 7 years ago

Tests are failing now, I cannot really understand why but there's something to dig here (run them locally with npm run dev)

TehShrike commented 7 years ago

It's because Travis doesn't expose the Saucelabs credentials to branches created by people who don't own the repo.

Either you'll have to run them yourself directly, or you can push this branch up to another branch in this repository and open a new pull request, at which point Travis should pass the credentials along happily.

vvo commented 7 years ago

I did run them myself and they are failing, just added one fix (cc @tzi we broke this contains method)

vvo commented 7 years ago

Only two failing tests now: image

The testing stack is only good in dev mode for now (travis and saucelabs are too brittle and may need an update on the tested browsers).

vvo commented 7 years ago

@tzi At some point you opened this https://github.com/vvo/in-viewport/pull/9

That's the tests that are failing so I am wondering if this current PR still works for you?

tzi commented 7 years ago

@vvo You totally right. I didn't make the link... Sorry :disappointed:

In theory, this library should not test visibility, but only test if an element is in the viewport. So this PR seems great, but if you try the example example/invisible-element.html it will always find a display: none element as in the Viewport.

That's a problem. For example, if you want to lazyload images stacked in horizontal containers AND to lazyload the containers stacked vertically (like the netflix UI). The images that are in not-already-loaded containers could load because there are not displayed.

We should actually replace the old isVisible method in isDisplayed. Here is a proposition with a complementary example https://github.com/tzi/in-viewport/commit/8fff59e8c2825c0e35ca5cc3dd053daa670f0f7d @cusspvz What do you think?

Cheers, Thomas.