-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Call scrollTo function when scrollViewOffset changed #4048
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing that I wonder (and posted short comment inline about this somewhere), is if this could be done using mutable's value listeners instead of overriding the setter. In that scenario we'd just use a standard shared value after which we schedule runOnUI call that sets up the listener (you can do SV.addListener(clb) that gets called when the value is updated). If that's possible it'd require less duplication, no modification of the core elements, and less code overall.
Secondly, I find this API a bit cumbersome: you need to create animated ref, then use that ref with one another hook, and then pass ref to Animated.ScrollView. I'd like to see a better example with this being used actually. The example you provided only updates the position but never reads from it. For this scenario one can just use synchronous scrollTo methods with animated-ref. Once we have a good example I'd be happy to provide better feedback on the API itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for the implementation I don't have more comments at this point. I think it is ok with just one minor comment I added for a code that's been there before (maybe we could change it here too).
I do have some comments, however, related to the API design of the feature. I commented earlier that I find it a bit too cumbersome and would like to if you could try to come up with some alternative approach. Because scroll view offset will only be supported with the wrapped ScrollView (Animated.ScrollView) we can potentially extend the parameter list and add some new parameter to facilitate this feature as opposed to just relying on refs which just doesn't feel super convinient for this usecase.
Summary
When position of the Scroll View is changed,
scrollViewOffset
value is updated.In this PR we would like to add opposite - when
scrollViewOffset
value is changed, we move the Scroll View to appropriate position.useScrollViewOffset
sharedValue
object to prevent scrolling in the loopTest plan
Testing on example screen