scroll-data-hook icon indicating copy to clipboard operation
scroll-data-hook copied to clipboard

WIP Allow attaching scroll data hook to any element

Open sanbornhilland opened this issue 4 years ago • 2 comments

@dejorrit This is extremely useful and solved some tricky scroll behaviour I was struggling with, so kudos for creating this hook. The one thing is that I have to modify the hook slightly for my use-case because I need to attach to a DOM element instead of the window. From initial quick tests it works with pretty minimal changes to accept a ref instead of assuming the window. I'd rather submit back to this project than permanently forking if possible so I'm wondering if you're open to supporting this option? My proposal would be to add an optional ref in the config. If it's not provided than behaviour would remain unchanged but if it is provided then the scroll data would be collected from the provided element instead. This draft doesn't have the config option, it's just a proof of what changes would be required to handle DOM elements instead of the window.

sanbornhilland avatar Dec 03 '20 18:12 sanbornhilland

Hey @sanbornhilland thanks a lot for coming up with this! To be honest, I knew this use case would be needed at some point :)

So feel free to continue your work on this PR to make it support both window and refs. Would be very nice if we could add this functionality!

dejorrit avatar Dec 03 '20 21:12 dejorrit

@dejorrit I have a code sandbox I've been using here if you want to easily play with changes: https://codesandbox.io/s/usescrolldatamod-j5xye?file=/src/App.js. You can swap between attaching the window and attaching to the ul by commenting/uncommenting the lines with the ul ref (55, 66, 98).

One thing I noticed is that if use the window but scroll the scroll container, scrolling and milliseconds update. That seems like a bug to me. It should probably be checking the target to make sure that it's only capturing scroll events that originated in the right place. What do you think? That issue exists in the latest release so probably it would make sense to do as a separate fix.

Let me know what you think about the direction of this PR. I'll squash everything before this gets merged.

sanbornhilland avatar Dec 04 '20 18:12 sanbornhilland