search

stratiformltd / react-loadable-visibility

A wrapper around react-loadable and @loadable/component to load elements once they are visible on the page.
BSD 3-Clause "New" or "Revised" License
1.01k stars 31 forks source link

#18 Add `isIntersecting` attribute fallback for zero threshold #19

Closed Zubrolet closed 5 years ago

Zubrolet commented 5 years ago

We can't just use entry.intersectionRatio >= 0 because it will lead to false positives, instead, we can add isIntersecting attribute as a second conditional.

To be honest, there is a chance that using only isIntersecting would be enough.

tazsingh commented 5 years ago

Hey sorry for the delay. I'm just getting back from vacation and will be able to take a deeper look at this and other issues on Monday.

At face value, this looks ok. Can't recall if there are tests that are dependent on this however and will have to double check if this doesn't conflict with other proposed solutions in other PRs/issues.

Thanks for the PR and will get back to you soon!

Zubrolet commented 5 years ago

Yeah, that's okay. I've already looked at other PR and it seems, that conflicts will not be so difficult to solve, and I'm ready to solve them. BTW, I tried to come up with some tests for this hotfix, but it's quite hard. I could make a simple snippet in smth like online code editor to demonstrate the actual behaviour. I did it for my work project and it seems that everything is fine.

tazsingh commented 5 years ago

@Zubrolet Looks like you should be able to work with the makeElementsVisible call that exists in the mock right now: https://github.com/stratiformltd/react-loadable-visibility/blob/0c7d3f4370aff7d54958dddfde4d9d6eb6307c18/src/__mocks__/IntersectionObserver.js#L6-L17

Would need to provide the isIntersecting property in there as well with various different values to express the behaviour of the code you've written.

Happy to give you a hand with this if you're having difficulty or pick it up instead! Let me know.

Zubrolet commented 5 years ago

@tazsingh I didn't know how exactly should it look like, so, how about this variant? If you meant something else, then feel free to contact me directly.

P.s. and just for the context, here is the Pen with the example of browser behaviour: https://codepen.io/Zubrolet/pen/QzwpjR

tazsingh commented 5 years ago

Nice! Thanks for the fix and the Codepen. That was helpful to demonstrate the issue.

I'll release this shortly 👏