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

Make scrolling other elements trigger lazy-loading #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zaccharles
Copy link

This PR fixes a problem I currently have. Simply, I have a div on my page which is overflow: auto. Inside this div, I have a bunch of images that I want to lazy-load with Layzr. The issue is that Layzr's handlers method only operates on window. This means that only scrolling the window itself will trigger lazy-loading, and scrolling any other element won't. I propose the addition of handlersFor which will add the scroll handler for the given node.

Note: I looked into detecting resizing of a div (though, it's not a problem I have). It seems that this is a popular solution, and very efficient. However, it would introduce a dependency for an edge case which is probably rare. Having said that, maybe we should expose requestFrame (perhaps under a different name) as a hook for adding other triggers?

@callmecavs
Copy link
Owner

@zaccharles I like the idea. I think for a cleaner implementation though, it might make more sense to extend the current handler method. If it accepted an additional parameter, that was a selector or element node (and defaulted to the window object if not passed in), it would keep the code DRYer, and accomplish the same thing.

I'll look into this in the near future, when I get some time to potentially cut a release. I also want to do some testing, to make sure the existing in viewport calculation will work for elements that are inside scrolling containers, rather than a scrolling window.

@zaccharles
Copy link
Author

I think that sounds like a better solution.

As for the in viewport calculation, I can tell you that it worked on my app, so at least there's some promise :) I also exposed requestFrame in my fork and found it quite useful to have access the that.

Anyway, I'll continue using my fork for now, happy to help if I can, though I think you're all over it :)

@callmecavs
Copy link
Owner

cheers man 🍻 hopefully I'll get a chance to take a look this weekend - if not definitely next week. glad to hear that this is working in your fork, praying I wont have to rework anything haha

@iztsv
Copy link

iztsv commented Feb 8, 2017

Hello @callmecavs and @zaccharles
As I can see this branch has conflicts.

@zaccharles are you planning to resolve conflicts?

Or in another way I can create new pull request for the same feature with my code and tests. What do you think about it?

@zaccharles
Copy link
Author

Happy for you to create a new PR @ilyaztsv :)

@iztsv
Copy link

iztsv commented Feb 10, 2017

@zaccharles I created PR regarding to optional container with scroll.

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.

3 participants