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

feat: Add simultaneousWithExternalGesture to ReanimatedSwipable #3324

Conversation

MrSltun
Copy link
Contributor

@MrSltun MrSltun commented Jan 10, 2025

Description

This PR adds a new prop to RenimatedSwipable that allows adding a gesture config that can be used simultaneously with ReanimatedSwipeable's gesture config.

The new prop is called simultaneousWithExternalGesture, it uses the the cross-component interaction methodology that's mentioned in the documentation here.

I also update the docs for it

Motivation

I've been recently using ReanimatedSwipeable component, and I tried to add an additional gesture config to enable haptic feedback and "release to run function" kind of interaction

After looking through the issues in the repo, I found this issue #2862, I tried to apply it, but it turned out that it was only applicable to the Swipable component

I tried to find a way to add a gesture config that would work simultaneously with the swipeable one but I couldn't find a way to add it. So I looked through the documentation of RNGH for simultaneous gesture handlers and came up with this solution

Test plan

I already tested it locally on both iOS and Android, and the callbacks work as expected

Not sure if there's something else to do here since this is my first contribution

@latekvo
Copy link
Contributor

latekvo commented Jan 15, 2025

I think this is very reasonable.

One thing i'd request, is that you rename the prop to simultaneousWithExternalGesture, as we already use simultaneousWithExternalGesture for this functionally identical purpose, more info here.

I'm not sure if we want to also add requireExternalGestureToFail and blocksExternalGesture, since user usually has access to these function on both the parent, and the child, while this implementation would only allow the user to use them on the child (Swipeable) and not the parent (Gesture wrapping the Swipeable).
I think we should wait for @m-bert's and @j-piasecki's opinion on this.

@m-bert
Copy link
Contributor

m-bert commented Jan 16, 2025

Hi @MrSltun! Thanks for this PR!

One thing i'd request, is that you rename the prop to simultaneousWithExternalGesture, as we already use simultaneousWithExternalGesture for this functionally identical purpose, more info here.

Actually I'm not sure if we want to rename it in current state. Also, now when I look at it, I assume that ReanimatedSwipeable is wrapped with GestureDetector, which means that simultaneousGesture is used inside another GestureDetector. For that cases we have simultaneousWithExternalGesture prop, as combining gestures in this way may not work.

What I think should be done here is to call this prop simultaneousWithExternalGesture and use it inside:

    const panGesture = useMemo(
      () =>
        Gesture.Pan()
          .enabled(enabled !== false)
          .enableTrackpadTwoFingerGesture(enableTrackpadTwoFingerGesture)
          .activeOffsetX([-dragOffsetFromRightEdge, dragOffsetFromLeftEdge])
          .onStart(() => {
            updateRightElementWidth();
          })
          .onUpdate(
            (event: GestureUpdateEvent<PanGestureHandlerEventPayload>) => {
              userDrag.value = event.translationX;

              const direction =
                rowState.value === -1
                  ? SwipeDirection.RIGHT
                  : rowState.value === 1
                    ? SwipeDirection.LEFT
                    : event.translationX > 0
                      ? SwipeDirection.RIGHT
                      : SwipeDirection.LEFT;

              if (!dragStarted.value) {
                dragStarted.value = true;
                if (rowState.value === 0 && onSwipeableOpenStartDrag) {
                  runOnJS(onSwipeableOpenStartDrag)(direction);
                } else if (onSwipeableCloseStartDrag) {
                  runOnJS(onSwipeableCloseStartDrag)(direction);
                }
              }

              updateAnimatedEvent();
            }
          )
          .onEnd(
            (event: GestureStateChangeEvent<PanGestureHandlerEventPayload>) => {
              handleRelease(event);
            }
          )
          .onFinalize(() => {
            dragStarted.value = false;
          })
+       .simultaneousWithExternalGesture(simultaneousGesture),
      [
        dragOffsetFromLeftEdge,
        dragOffsetFromRightEdge,
        dragStarted,
        enableTrackpadTwoFingerGesture,
        enabled,
        handleRelease,
        onSwipeableCloseStartDrag,
        onSwipeableOpenStartDrag,
        rowState,
        updateAnimatedEvent,
        updateRightElementWidth,
        userDrag,
      ]
    );

Could you please check if this works?

@MrSltun
Copy link
Contributor Author

MrSltun commented Jan 17, 2025

Actually I'm not sure if we want to rename it in current state. Also, now when I look at it, I assume that ReanimatedSwipeable is wrapped with GestureDetector, which means that simultaneousGesture is used inside another GestureDetector. For that cases we have simultaneousWithExternalGesture prop, as combining gestures in this way may not work.

My Approach was just to combine an external gesture with the ReanimatedSwipeable's GestureDetector (without a parent GestureDetector wrapping the ReanimatedSwipeable)

But I like the approach of having the ReanimatedSwipeable gesture working simultaneously with a parent GestureDetector, I will update the code, test it, and report back here :)

Edit: It works perfectly! I'll update the PR now!

@MrSltun MrSltun changed the title feat: Add simultaneousGesture to ReanimatedSwipable feat: Add simultaneousWithExternalGesture to ReanimatedSwipable Jan 17, 2025
@latekvo
Copy link
Contributor

latekvo commented Jan 17, 2025

Works great, test code used:

Collapsed repro
import React from 'react';
import { View, Text } from 'react-native';
import { Gesture, GestureDetector } from 'react-native-gesture-handler';
import Swipeable from 'react-native-gesture-handler/ReanimatedSwipeable';

export default function Example() {
  const pan = Gesture.Pan()
    .onBegin(() => console.log('began'))
    .onStart(() => console.log('active'))
    .onFinalize((_, success) => console.log(success ? 'ended' : 'cancelled'));

  return (
    <GestureDetector gesture={pan}>
      <Swipeable
        leftThreshold={50}
        rightThreshold={50}
        renderLeftActions={() => {
          return (
            <View style={{ height: 80, width: 80, backgroundColor: 'yellow' }}>
              <Text>Left</Text>
            </View>
          );
        }}
        renderRightActions={() => {
          return (
            <View style={{ height: 80, width: 80, backgroundColor: 'magenta' }}>
              <Text>Right</Text>
            </View>
          );
        }}>
        <View
          style={{
            height: 80,
            backgroundColor: 'blue',
          }}
        />
      </Swipeable>
    </GestureDetector>
  );
}

Just one thing - the Tap in Swipeable isn't made simultaneous with the provided simultaneousWithExternalGesture prop. Unless I'm missing something, I think it should also be made simultaneous.

Also, I'm getting a type error when i pass my pan gesture to the simultaneousWithExternalGesture in the test code I provided, but as far as I see, GestureType is the correct type to use in this scenario. I'm not sure if this is a case of package version discrepancy or some other issue?
image

@MrSltun
Copy link
Contributor Author

MrSltun commented Jan 17, 2025

Thank you for testing the changes @latekvo 🫶

Just one thing - the Tap in Swipeable isn't made simultaneous with the provided simultaneousWithExternalGesture prop. Unless I'm missing something, I think it should also be made simultaneous.

Hmm I tested the pan gestures, but didn't check the other types, I'll test all the gestures and see which works and which doesn't, and try to figure out why

Also, I'm getting a type error when i pass my pan gesture to the simultaneousWithExternalGesture in the test code I provided, but as far as I see, GestureType is the correct type to use in this scenario. I'm not sure if this is a case of package version discrepancy or some other issue?

Oh that's weird! I didn't come across it, not sure why it's happening but I'll check

@m-bert
Copy link
Contributor

m-bert commented Jan 20, 2025

TypeScript error

We've spent some time with @tjzel trying to find out what's going on. Turns out that this is a problem with our configuration (thank you @j-piasecki ❤️).

Could you please update tsconfig.json in our example app? Otherwise CIs may fail

{
  "extends": "../tsconfig.json",
  "compilerOptions": {
    "noEmit": true,
    "baseUrl": ".",
    "paths": {
      "react-native-gesture-handler": ["../src/index.ts"],
+     "react-native-gesture-handler/ReanimatedSwipeable": [
+       "../src/components/ReanimatedSwipeable.tsx"
+     ],
      "react-native-gesture-handler/jest-utils": ["../src/jestUtils/index.ts"]
    }
  },
  "include": ["src/**/*.ts", "src/**/*.tsx", "App.tsx"]
}

Types

Regarding the code, I'd like to use the type that simultaneousWithExternalGesture expects, i.e.

simultaneousWithExternalGesture: Exclude<GestureRef, number>[];

And that requires a change in passing it into config:

.simultaneousWithExternalGesture(
  ...(simultaneousWithExternalGesture ?? [])
),

In that case, example usage looks like this:

<ReanimatedSwipeable
  simultaneousWithExternalGesture={[pan, tap]}
  ...
>

I know that this may be inconvenient with single gestures - we can change the type to:

simultaneousWithExternalGesture: Exclude<GestureRef, number> | Exclude<GestureRef, number>[];

This will require a bit more logic when passing this prop into config, but it will be easier to use 😅

@MrSltun MrSltun force-pushed the mrsltun/feat/allow-simultaneous-in-reanimated-swipeable branch from ee0392a to c30bbe2 Compare January 27, 2025 10:26
@MrSltun
Copy link
Contributor Author

MrSltun commented Jan 27, 2025

@m-bert thank you for taking the time to test the change :)

I updated the the example app's tsconfig

Re: updating the simultaneousWithExternalGesture type,
I like the new approach since it allows multiple gestures to be passed to the ReanimatedSwipeable gesture handler :)

I updated the name as well to reflect that change to be simultaneousWithExternalGestures

I also tested the new change and it works as it should 🎉

Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

Hi @MrSltun! Thank you for changing our tsconfig 😅

Prop name

Please change prop name to simultaneousWithExternalGesture. I know that it sounds better with s at the end, but this is how it is currently named in Gesture Handler and we would like to stick to it.

Prop type

After looking at it I'd prefer to remove array if not necessary. It means that type of the prop should be

simultaneousWithExternalGesture?:
  | Exclude<GestureRef, number>
  | Exclude<GestureRef, number>[];

To handle it correctly, I suggest adding the following logic into useMemo:

      ...
        .onFinalize(() => {
          dragStarted.value = false;
        });

      if (!simultaneousWithExternalGesture) {
        return pan;
      }

      if (Array.isArray(simultaneousWithExternalGesture)) {
        pan.simultaneousWithExternalGesture(...simultaneousWithExternalGesture);
      } else {
        pan.simultaneousWithExternalGesture(simultaneousWithExternalGesture);
      }

      return pan;

Tap gesture

Currently this prop is not used with Tap gesture that is present inside Swipeable - please add this 😅

* An array of gesture objects containing the configuration and callbacks to be
* used with the swipeable's gesture handler.
*/
simultaneousWithExternalGestures?: Exclude<GestureRef, number>[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
simultaneousWithExternalGestures?: Exclude<GestureRef, number>[];
simultaneousWithExternalGesture?: Exclude<GestureRef, number>[];

@MrSltun
Copy link
Contributor Author

MrSltun commented Jan 27, 2025

@m-bert Thank you for the detailed feedback :)

I completely missed the tap gesture 😅 thank you for pointing it out!

I just updated the name of the variable, as well as the definition of the pan gesture, and I added the gesture to the tab one as well :)

@MrSltun MrSltun requested a review from m-bert January 27, 2025 14:23
Copy link
Contributor

@m-bert m-bert left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR @MrSltun ❤️

@m-bert m-bert merged commit 5b535c9 into software-mansion:main Jan 27, 2025
4 checks passed
@MrSltun MrSltun deleted the mrsltun/feat/allow-simultaneous-in-reanimated-swipeable branch January 27, 2025 16:07
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