Skip to content
This repository has been archived by the owner on Jan 14, 2020. It is now read-only.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cusspvz
Copy link

@cusspvz cusspvz commented Jun 20, 2017

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
Copy link
Owner

vvo commented Jun 20, 2017

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
Copy link
Collaborator

tzi commented Jun 20, 2017

For me, you summarized well the problem.

👍 to merge, and to release a major version

@vvo
Copy link
Owner

vvo commented Jun 20, 2017

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

@TehShrike
Copy link

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
Copy link
Owner

vvo commented Jun 20, 2017

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

@vvo
Copy link
Owner

vvo commented Jun 20, 2017

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
Copy link
Owner

vvo commented Jun 21, 2017

@tzi At some point you opened this #9

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

@tzi
Copy link
Collaborator

tzi commented Jun 21, 2017

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

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 tzi@8fff59e
@cusspvz What do you think?

Cheers,
Thomas.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants