Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add 'scrollStopped' event #103

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

Conversation

websirnik
Copy link

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.

- Rename duScrollSpyWait -> duScrollThrottle
- Add "duScrollspy:scrollStopped"
@oblador
Copy link
Owner

oblador commented Jan 27, 2015

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?

@websirnik
Copy link
Author

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.

@websirnik
Copy link
Author

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?

@oblador
Copy link
Owner

oblador commented Feb 6, 2015

@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 :-)

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

Successfully merging this pull request may close these issues.

2 participants