-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add web implementation for useScrollViewOffset #5805
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
Add web implementation for useScrollViewOffset #5805
Conversation
tjzel
left a comment
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.
The level of branching in this hook is high - let's split it into two separate implementations and export the relevant one, see ViewDescriptorSet.ts for reference.
tjzel
left a comment
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.
![]()
| export const useScrollViewOffset = IS_WEB | ||
| ? useScrollViewOffsetJS | ||
| : useScrollViewOffsetNative; |
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.
Should be useScrollViewOffsetWeb instead of useScrollViewOffsetJS
| const offsetRef = useRef( | ||
| // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| initialRef !== undefined ? initialRef : useSharedValue(0) | ||
| ); |
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.
Do we really need to call useSharedValue inside and ignore rules of hooks?
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.
This is exactly the situation we mentioned today, I made it this way so it mirrors the native implementation
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.
We want to either return values to a given sharedValue - initialRef, or return a new sharedValue that gets the updates. So far, no one came up with a better way to do it other than conditionally calling the hook. As soon as we find it or change this logic, it will get updated.
| // We need to make sure that listener for old animatedRef value is removed | ||
| if (scrollRef.current !== null) { | ||
| (scrollRef.current as unknown as HTMLElement).removeEventListener( | ||
| 'scroll', | ||
| eventHandler | ||
| ); | ||
| } |
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.
Why do we need to call removeEventListener here as well instead of only in useEffect cleanup function?
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.
@tomekzaw because the animatedRef reference might change to another scroll and we then want to clean the previous listener (see example in code section of PR description)
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.
Can we just assign const element = animatedRef.current first (when calling addEventListener) and then call removeEventListener on element?
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.
That's exactly what scrollRef does here
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.
To be precise: we store a ref to the affected component in scrollRef each time useEffect runs. Since its dependencies are animatedRef, animatedRef.current and eventHandler (which also regenerates only when animatedRef and animatedRef.current change), it re-runs ONLY when we switch the hook to another component. In such situation, cleanup on scrollRef runs first, ensuring there is no listener to a scroll that we don't care about, and then we can safely update scrollRef and add new listener to a new component.
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.
Oh, I see now. Anyway, is it possible to modify the code so that removeEventListener appear only once to make this useEffect "symmetrical" as recommended?
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.
I'm afraid I don't see any other way. The removeEventListener calls match two different scenarios - the scrollRef one makes sure we disconnect from an older scroll component during a switch, while the return cleanup runs when the component (or its parent) gets dismounted for good.
Summary
Our
useScrollViewOffsethad no support for web, so I added it using some basic web scroll listeners.There are some funny typing shenanigans, if anyone comes up with better solutions I will happily use it.
beforeoffset.mov
afteroffset.mov
Test plan
Check
useScrollViewOffsetexample from Reanimated WebExample and watch console logs or copy example code from our docs.More in-depth testing can be done using the following code - it tests switching and mouting/dismounting components that the hook refers to:
Code