-
-
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
Add web implementation for useScrollViewOffset #5805
Add web implementation for useScrollViewOffset #5805
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.
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.
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
useScrollViewOffset
had 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
useScrollViewOffset
example 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