Use 'scroll' and 'resize' DOM events instead of a timer

[mvila]

Instead of using a timer it would be great (and CPU saver...) to use 'scroll' and 'resize' DOM events to check the visibility only when it's necessary. https://github.com/Pomax/react-component-visibility does that but it doesn't have the 'containment' feature.

[joshwnj]

What I'd like here (but haven't gotten around to yet) is to use both. So we'd have scroll and resize listeners as the main trigger, but also have a timer for the cases where visibility changes without either a scroll or a resize.

With this implemented we'd treat the timer as a fallback and use a much longer delay, making the CPU impact negligible (or, at least: within the control of the developer).

Fancy having a go at that? PRs welcome :)

[mvila]

Absolutely, to use both a timer and events would be great. And in this case, perhaps the default 1000 ms delay is fine. Unfortunately, I'm too busy now to help you with the implementation. I hope someone can do it! :)

[joshwnj]

I've put a call out on twitter in case anyone would like to volunteer :) otherwise I'll get to it when I next have time

[Ghostavio]

I can take a look at it, if it's still something you guys want to add :)

[kof]

Or we remove both timer and don't add events. It can be easily done in the user space. I use active=false and do the checks manually in my code. Only user code can really know what it needs to react to and how often.

[mvila]

@Ghostavio: 👍 :)

@kof: If you remove everything, what would be the point of the module? :)

[kof]

I don't remove "everything", component remains as usefull as before, its a very small api change that makes api lower lever but with less bugs.

[kof]

you want periodical check? its easy

setInterval(::this.refs.sensor.check, 1000)

you want on scroll?

window.addEventListener('scroll', ::this.refs.sensor.check)

you want a scroll with debounce? (you should want it)

import debounce from 'lodash/function/debounce'
window.addEventListener('scroll', debounce(::this.refs.sensor.check, 200))

[kof]

everything you might want to do you can do just in one line of code, its the same line you will use for a fragile option.

[kof]

Also I would think about how to get rid of the imperative .check() call.

[mvila]

@kof: I think a sensor should be able to work by itself. Imagine if you had to call a check() method on a button to find out if it is clicked or not, it would be wrong don't you think?

[kof]

@mvila in the ideal world you would be right .... in practice I can see lots of issues to make it magically work.

If a tool can't do the magic without side effects, it should not have this magic.

[mvila]

@kof: Well, listening to scroll and resize events to trigger the visibility check doesn't look too magical to me. If we do that we can increase the timer to 1 sec or remove it completely. It should work in 95% of cases and for the remaining 5% the developer has always the possibility to trigger the checking manually.

[kof]

I know following problems:

1. List of items, where every item is wrapped into sensor. If we do it with timer, it should be just one for all instances, otherwise even with 1sec timer its hundreds of timers. Same for scroll event listener.
2. Problem with scroll event listener - it doesn't bubble up to the window or document when some inner container is scrolled. This means we need to attach the listener to the containment element we already have. In case of a list with lots of sensors we run again into listeners amount problem.
3. Resize event is only possible on window anyways. Container resizes can't be checked as far as I know. We can only try dom mutation observers, which is not always an indication for a resize.

[kof]

I could think of a container component which contains the listeners/timers and communicates with the inner sensors.

[joshwnj]

I like the idea of 2 separate components, @kof gives some good examples of why you might need that.

What if we said that by default the 2 components come bundled together, but you can opt into separating them and manually associating the trigger with the sensor?

[kof]

Yes its possible. Component "Sensor" and component "Trigger" and a default component which is Sensor wrapped into Trigger.

[botverse]

I agree with @mvila. The component would aim to be too generic removing all the setup, scroll and resize should be right for the majority of the cases.

As suggested by @kof, for particular cases that differ from that, active={false} and manually manage check is an acceptable solution.

@joshwnj active={false} is already opting out. Would make little sense to have two components for that.

So it seems just right to change the timer by debounced scroll/resize.

[Andarist]

https://developers.google.com/web/updates/2016/04/intersectionobserver?hl=en

That would be even more cool, ofc browser support sucks yet.

[kof]

Cool, I would rename this project to react-intersection-observer and start using IntersectionObserver if supported.

[Andarist]

Maybe (and that is strong maybe) I will have some time next week to give it a shot at implementing and testing this.

[eek]

Implemented this in #54 if you guys can have a look, it would be awesome 😁

[joshwnj]

Thanks @eek ! Will take a look