angular-scroll

angular-scroll copied to clipboard

Thanks for this directive, I've been using it for some time, super valuable! I've had one use case that I've thought might be relevant to include in the directive.

I needed to lazy-load a bunch of content once user starts scrolling. It's not good to do DOM manipulations while user scrolls, as it affects scrolling performance. Instead, once a user stops scrolling, it's a good time to execute DOM manipulation. And with the ScrollSpy I know where the scroll position is and can lazy-load just the right content.

This pull request broadcasts duScrollspy:scrollStopped event once user stops scrolling.

The summary of changes:

• Added duScrollspy:scrollStopped event
• Added duScrollDebouce constant - controls debouncing timeout. Default: 0 - Debounce is disabled, no event will fire.
• Breaking Change! Renamed duScrollSpyWait constant -> duScrollThrottle. (Feels it's more appropriate naming).

If you are happy with it, I can add it to the docs.

Thanks for the PR!

Two notes:

• This is pretty generic events that doesn't necessarily have anything to do with scroll spies. Maybe this is better to have in a submodule of it's own so that the amount of events isn't multiplied with number of contexts?
• Why have two constants for the same type of functionality?

I think you are right. It is a generic event and a separate module is a good idea.

In the current implementation, having 2 constants allows to disable funcitonalities separetly, either the interval for ScrollSpy checks or broadcast of duScrollspy:scrollStopped event.

I think if it would be a separate module, there might be no need for 2 constants.

Let me refactor it into a separate sub-module. I think placing a directive du-scroll-notify on a scrollable container is a good approach.

I've refactored the code. Here is a summary of changes:

• New modules: duScroll.scrollNotify & duScroll.notifyAPI
• New directive: du-scroll-notify - you would place it on scrollable element. Listener broadcasts duScrollspy:scrollStopped event when user stops scrolling.

The implementation of the directive and service is very simple, it covers a use case with one scrollable container. Good idea down the road would be to supoprt multiple containers. I wasn't sure how to achive this with minimal impact, could just duplicate the logic of SpyAPI with multiple contexts and containers. (I didn't fully understand the logic to be honest. May be adding inline explanatory comments would help). But I guess it might be better to add one common dependeny to spyAPI & notifyAPI to implement contexts and containers for both.

@oblador What are your thoughts?

@websirnik Thank you for your work!

Just a thought, how do you feel about making it a bit more declarative and contextual with this syntax du-scroll-stopped="ctrl.fn()". We could keep the event as well for more generic uses. I can implement it, no worries :-)