react-scroll-horizontal
react-scroll-horizontal copied to clipboard
hew
onScroll, onReachStart, onReachEnd callbacks
I found react-scroll-horizontal very useful, but in my use case I needed some more control over the component and I wanted to run callbacks on reaching the scrolling area boundaries. I've made a few changes, that maybe you find reasonable enough to merge them to the repository.
This PR introduces onScroll, onReachStart and onReachEnd callbacks and changes also a little bit of existing logic, as described below.
if (prevProps.animValues !== this.props.animValues) {
let currentAnimValues = this.state.animValues
this.setState({
animValues: currentAnimValues + this.props.animValues
}, this.calculate())
}
Controlling scroll position by a relative value seems a bit limited to me. My feeling is that if a component can be controlled from the parent level, it should be fully controlled by an absolute value (and return current position to onScroll callback). It gives possibility to set scroll position exactly where you want to (at the start, at the end, to some element). So I've changed this setState to animValues: this.props.animValues.
However, I'm aware that this is kind of a "breaking change" and that scrolling by relative value may be good for some use cases. I'm thinking about keeping these two ways of controlling the scroll position maybe?
if (curr >= 1) {
this.resetMin()
} else if (curr <= bounds) {
var x = bounds + 1
this.resetMax(x)
}
I'm not sure what was the purpose of 1 and +1, but I discovered that if scroll position is controlled by the animValues property and you set limit values (0, max), newly introduced onReachStart and onReachEnd (placed in resetMin and resetMax) wouldn't be called. I haven't noticed any issues after changing this part, but if I did some wrong here and should call these callbacks somewhere else, then please let me know.
@kwieccia I love these changes ❤ and the overall thinking here.
I'm super busy right now, so I can't say when I'll merge this, but I certainly intend to.
OK I had some time to dig into this. Some thoughts:
My feeling is that if a component can be controlled from the parent level, it should be fully controlled by an absolute value (and return current position to onScroll callback). It gives possibility to set scroll position exactly where you want to (at the start, at the end, to some element).
I like this. I'm fine this change, so long as we:
- Provide a fallback
- Can achieve a decent balance of performance and responsive.
Regarding point #2, there are not many perf optimizations going on right now, so I'm cautious about doing more work on the scroll event listener. But I'm open to it.
if (this.props.onReachStart) {
this.props.onReachStart()
}
We'll likely want to throttle these calls a bit. The UX of this is such that once you hit the end/start, you are almost always still scrolling, so this will likely result in a bunch of callbacks firing all at once. The throttle speed could maybe be another prop.
Hi @hew,
Thank you for the feedback! I finally found some time to make changes.
1. Provide a fallback
I've restored the logic of animValues to keep legacy and not lose this functionality.
The prop introduced by me has been renamed to scrollToValue.
Now it's possible to emulate scroll however you want (by relative or absolute value).
2. onReach callbacks performance I added lodash throttle and indeed, multiple function calls have been limited. I surely can also add throttle speed as a prop, but I don't know if it would be very useful. 800ms seems to be pretty fine timeout in terms of UX and scroll movement, so do you think it's worth controlling? (I'm just afraid that too many props listed in documentation can be overwhelming.)
Yeah I was pretty on-the-fence about the throttle prop. I'm totally cool to leave it as-is for now.
Thanks for these contributions @kwieccia! I'll see what I can do about getting them merged in relatively soon.
