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

Add web implementation for useScrollViewOffset #5805

Merged
merged 4 commits into from
Mar 20, 2024

Conversation

szydlovsky
Copy link
Contributor

@szydlovsky szydlovsky commented Mar 18, 2024

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.

before after
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
import React, { useState } from 'react';
import Animated, {
  useAnimatedRef,
  useDerivedValue,
  useSharedValue,
  useScrollViewOffset,
} from 'react-native-reanimated';
import { Button, StyleSheet, Text, View } from 'react-native';

export default function ScrollViewOffsetExample() {
  const aref = useAnimatedRef<Animated.ScrollView>();
  const bref = useAnimatedRef<Animated.ScrollView>();
  const scrollHandler = useSharedValue(0);
  const [scrollAMounted, setScrollAMounted] = useState(true);
  const [scrollBMounted, setScrollBMounted] = useState(true);
  const [scrollAPassed, setScrollAPassed] = useState(true);

  useDerivedValue(() => {
    console.log(scrollHandler.value);
  });

  const onAMountPress = () => {
    setScrollAMounted(!scrollAMounted);
  };
  const onBMountPress = () => {
    setScrollBMounted(!scrollBMounted);
  };
  const onPassTogglePress = () => {
    setScrollAPassed(!scrollAPassed);
  };

  useScrollViewOffset(scrollAPassed ? aref : bref, scrollHandler);

  return (
    <>
      <View style={styles.positionView}>
        <Text>Test</Text>
      </View>
      <View style={styles.divider} />
      <Button
        title={`${scrollAMounted ? 'Dismount' : 'Mount'} scroll A`}
        onPress={onAMountPress}
      />
      <Button
        title={`${scrollBMounted ? 'Dismount' : 'Mount'} scroll B`}
        onPress={onBMountPress}
      />
      <Button
        title={`Toggle the ref, currently passed to ${
          scrollAPassed ? 'scroll A' : 'scroll B'
        }`}
        onPress={onPassTogglePress}
      />
      {scrollAMounted ? (
        <Animated.ScrollView
          ref={aref}
          style={[styles.scrollView, { backgroundColor: 'purple' }]}>
          {[...Array(100)].map((_, i) => (
            <Text key={i} style={styles.text}>
              A: {i}
            </Text>
          ))}
        </Animated.ScrollView>
      ) : null}
      {scrollBMounted ? (
        <Animated.ScrollView
          ref={bref}
          style={[styles.scrollView, { backgroundColor: 'lime' }]}>
          {[...Array(100)].map((_, i) => (
            <Text key={i} style={styles.text}>
              B: {i}
            </Text>
          ))}
        </Animated.ScrollView>
      ) : null}
    </>
  );
}

const styles = StyleSheet.create({
  positionView: {
    margin: 20,
    alignItems: 'center',
  },
  scrollView: {
    flex: 1,
    width: '100%',
  },
  text: {
    fontSize: 50,
    textAlign: 'center',
  },
  divider: {
    backgroundColor: 'black',
    height: 1,
  },
});

@szydlovsky szydlovsky requested a review from tjzel March 18, 2024 14:39
Copy link
Collaborator

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

@szydlovsky szydlovsky requested a review from tjzel March 19, 2024 10:34
@szydlovsky szydlovsky requested a review from tjzel March 20, 2024 14:16
Copy link
Collaborator

@tjzel tjzel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@szydlovsky szydlovsky added this pull request to the merge queue Mar 20, 2024
Merged via the queue into main with commit d6dab8f Mar 20, 2024
7 checks passed
@szydlovsky szydlovsky deleted the @szydlovsky/useScrollViewOffset-web-implementation branch March 20, 2024 15:13
Comment on lines +25 to +27
export const useScrollViewOffset = IS_WEB
? useScrollViewOffsetJS
: useScrollViewOffsetNative;
Copy link
Member

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

Comment on lines +33 to +36
const offsetRef = useRef(
// eslint-disable-next-line react-hooks/rules-of-hooks
initialRef !== undefined ? initialRef : useSharedValue(0)
);
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@szydlovsky szydlovsky Mar 20, 2024

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.

Comment on lines +49 to +55
// 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
);
}
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@szydlovsky szydlovsky Mar 20, 2024

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.

Copy link
Member

@tomekzaw tomekzaw Mar 20, 2024

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?

Copy link
Contributor Author

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.

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.

3 participants