Changed default behaviour from intervalCheck to onScroll Listener.

[eek]

Fixes #10 and #36

Update: Changed main behaviour from intervalCheck to onScroll Listener
Update: Delay props now referrer to the scrolling debounce time until check is called
Update: IntervalCheck still exists but needs to be enabled with a bool intervalCheck prop, and added intervalDelay prop (which was the old delay) that can adjust the interval check.
Update: Updated tests to the new version and added test for user scrolling.
Update: intervalCheck is automatically enabled when scrollCheck props is set to false
Update: Readme to add the new props + descriptions

[eek]

god, the tests failed on travis :(

Now I need to figure out why they work with Chrome and fail on PhantomJS

Later-Edit: Damn, apparently PhantomJS viewport increases with whatever is the document, so scrolling does nothing because the element will always be in viewport 😱

[eek]

Also changed Travis to run the tests with Chrome, so we have consistency between development machines and Travis.

[Andarist]

hm, im wondering if throttle wouldnt be a better fit here instead of debounde, wdyt?

[eek]

@Andarist, I initially implemented throttle but my problem was that usually the visibility state matters when you reach the end of scrolling, so throttle, with a delay of 250ms means that it triggers once the user starts scrolling and again every 250ms if the scrolling persists. I got weird results in my usage with Visibility Sensor when I plugged in throttle.

I think it's better to trigger at the end than at the start, a best implementation I think would be a combination of both.

[joshwnj]

@eek sorry it's taken me a while to get to reviewing this. I've had some time to sit down and look over this closely today, and really like what I see - thanks!

Some notes:

• nice move on setting up Chrome_travis_ci! TIL :)
• i'm going to keep intervalCheck as the default method (otherwise we break backwards-compatibility). I'd like to look at doing a major-version release in early 2017, and that would be a great time to make it scroll-check-by-default.
• I'm thinking it would be clearer to rename delay to scrollDelay, wdyt?
• at the moment the scroll event is always attached to the window, which means it won't work with the containment prop. I think I've got that working, so once I've pushed I'll ask for your review :)

[joshwnj]

We might also consider adding an option to switch between throttle or debounce, because I can imagine situations when one would make sense over the other.

[joshwnj]

@eek here's my proposal for a few changes to your PR. If you're happy with them I'll go ahead and merge, but let me know if you want to discuss anything: eek/react-visibility-sensor@master...joshwnj:pr-54-proposal

Summary of changes:

• keeping intervalCheck as the default behaviour, for backwards compatibility
• renamed new props to scrollCheck and scrollDelay
• updated the example to demonstrate a long-delay intervalCheck with a short-delay scrollCheck
• listen to scroll event on the appropriate container, rather than just the window

[eek]

Looks good! :D I'm ok with the changes :D Although I don't know how listening to scroll on a container behaves, I'm always attaching the scroll event to window / document :D But since the tests passed, that's awesome. :D

[Andarist]

What would you say about implementing IntersectionObserver support? It could even be the default with scroll and interval being used only as a fallback settings. Its already implemented in 2 browsers.

https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API

[joshwnj]

@Andarist great idea, I've created an issue for it: #56

[joshwnj]

Thanks for this pr @eek ! Published v3.6.0 to npm.

Cool to see how much more responsive the contained element is in the example too: http://joshwnj.github.io/react-visibility-sensor/