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 swipe-to-interact functionality to Swipeable component #10912

Conversation

olerichter00
Copy link
Contributor

@olerichter00 olerichter00 commented Oct 9, 2024

This PR resolves EMI-2127

Description

This PR adds swipe-to-interact functionality (swiping until the end without tapping on the action button) to the Swipeable component. We might fine-tune the interaction later after testing it on a real device.

Simulator.Screen.Recording.-.iPhone.15.Plus.-.2024-10-09.at.08.55.38.mp4

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • Add swipe-to-interact functionality to Swipeable component (not used yet) - ole

Need help with something? Have a look at our docs, or get in touch with us.


const FRICTION = 1.2
const SWIPE_TO_INTERACT_THRESHOLD = 80
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 the threshold up to which the distance from the screen edge the user has to swipe to trigger the action.

@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Oct 9, 2024

This PR contains the following changes:

  • Dev changes (Add swipe-to-interact functionality to Swipeable component (not used yet) - ole)

Generated by 🚫 dangerJS against b4eaa8c

Comment on lines +84 to +86
onLayout={(event) => {
width.value = event.nativeEvent.layout.width
}}
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 using the component's width to determine when the dragX of the drag interaction has reached the end of the component.

Copy link
Contributor

@araujobarret araujobarret left a comment

Choose a reason for hiding this comment

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

Not sure if I follow the need for this extra function, I think the Reanimated element already provides onSwipeableOpen, where we can control kinda the same of actionOnSwipe without any extra code. Then it's up to the caller to control it and the swipeable element stays simpler and more performant.

@olerichter00
Copy link
Contributor Author

Not sure if I follow the need for this extra function, I think the Reanimated element already provides onSwipeableOpen, where we can control kinda the same of actionOnSwipe without any extra code. Then it's up to the caller to control it and the swipeable element stays simpler and more performant.

Maybe my PR description wasn't very clear. This new functionality is about swiping until the end without tapping on the action button (The Jira ticket has a video and better description of the interaction). onswipeableopen is only called once when the user swipes and the buttons are displayed.

Copy link
Contributor

@araujobarret araujobarret left a comment

Choose a reason for hiding this comment

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

Ah it makes sense, not sure about the UX since there's nothing on the DS, looks legit just one thing we might consider to change.

progress: SharedValue<number>,
const width = useSharedValue(0)

const [hasSwiped, setHasSwiped] = useState(false)
Copy link
Contributor

Choose a reason for hiding this comment

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

this can be replaced by useRef from what I see in the code

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've tried to make it work but it doesn't.... Is this what you would suggest?

  const hasSwiped = useRef(false)

  const handleSwipeToInteract = () => {
    hasSwiped.current = true
  }

  useEffect(() => {
    if (!hasSwiped.current) return

    actionOnSwipe?.()
  }, [hasSwiped.current])

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that won't work because your useEffect comes from the state change reaction when we call `runOnJs. What could work is:

// no useEffect anymore
const handleSwipeToInteract = () => {
  hasSwiped.current = true
  actionOnSwipe?.()
}

// line 51
if (!actionOnSwipe || hasSwiped.current || !width.value) return {}

// line 56
runOnJs(handleSwipeToInteract)()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot, that's very clean! Of course, with the ref, there is no need for the effect anymore. I've updated the code 👍

@olerichter00
Copy link
Contributor Author

Ah it makes sense, not sure about the UX since there's nothing on the DS, looks legit just one thing we might consider to change.

You are right, the UX might change. The designs for the new home view's Tasks/Notifications section are still wip...

Comment on lines +35 to +38
const hasSwiped = useRef(false)

const handleSwipeToInteract = () => {
hasSwiped.current = true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To prevent calling the action more than once.

@damassi
Copy link
Member

damassi commented Oct 9, 2024

@olerichter00 - Is there any way to make it so that element doesn't just vanish? Looks a bit clunky just popping out like that.

In Moti, there's an <AnimatePresence> component that can be used to check if something is appearing and disappearing. Ie, if

<AnimatePresence>
  {foo && 
    <MotiView exit={{ ...someExitAnimation }} exitTransition={{ ... }}>

We should lean on that, even if its a simple opacity. But better: animate the height of the container to 0, which will transition the content under the swiped-out component up to the top. Should do what you need.

@olerichter00
Copy link
Contributor Author

olerichter00 commented Oct 9, 2024

@olerichter00 - Is there any way to make it so that element doesn't just vanish? Looks a bit clunky just popping out like that.

@damassi, yes, there are a couple of options like LayoutAnimation, Disappearable, HeighTransition, or Animated.Flastlist that can be used to animate the item disappearing.

Because it might depend on the context (e.g. list vs single item), I thought it would be good not to build the animation into the Swipeable component.

@damassi
Copy link
Member

damassi commented Oct 9, 2024

Ah, that makes sense 👍

@olerichter00 olerichter00 merged commit 489dea1 into main Oct 10, 2024
7 checks passed
@olerichter00 olerichter00 deleted the olerichter00/EMI-2127/add-swipe-to-interact-functionality-to-swipeable-component branch October 10, 2024 06:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants