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

Call scrollTo function when scrollViewOffset changed #4048

Closed

Conversation

mstach60161
Copy link
Contributor

@mstach60161 mstach60161 commented Feb 13, 2023

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.

  • Added listener in useScrollViewOffset
  • Added flag to sharedValue object to prevent scrolling in the loop

Test plan

Testing on example screen

  • Animated scroll
  • Static scroll
  • Cancel animations
  • Old functionality

@mstach60161 mstach60161 marked this pull request as ready for review February 21, 2023 11:52
Copy link
Member

@kmagiera kmagiera left a 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

Example/src/ScrollViewOffsetCallScrollToExample.tsx Outdated Show resolved Hide resolved
src/reanimated2/hook/useScrollViewOffset.ts Outdated Show resolved Hide resolved
src/reanimated2/scrollValueSetter.ts Outdated Show resolved Hide resolved
Copy link
Member

@kmagiera kmagiera left a 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.

src/reanimated2/hook/useScrollViewOffset.ts Show resolved Hide resolved
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