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(DIA-1015): allow FancySwipe to prevent left and/or right swiping #11357

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

iskounen
Copy link
Contributor

@iskounen iskounen commented Jan 8, 2025

This PR updates the FancySwiper component to make right swiping optional since Infinite Discovery will only allow users to swipe in one direction. (I also added the option to make left swiping optional for the sake of symmetry.)

Simulator.Screen.Recording.-.iPhone.16.Plus.-.2025-01-09.at.13.34.18.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

  • Allowed users of the FancySwiper to prevent right swipes

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

@iskounen iskounen requested a review from MounirDhahri January 8, 2025 18:59
@iskounen iskounen self-assigned this Jan 8, 2025
@iskounen iskounen marked this pull request as draft January 9, 2025 10:37
@iskounen iskounen marked this pull request as ready for review January 9, 2025 18:34
@ArtsyOpenSource
Copy link
Contributor

This PR contains the following changes:

  • Dev changes (Allowed users of the FancySwiper to prevent right swipes - iskounen)

Generated by 🚫 dangerJS against abd2876

const onSwipeHandler = (swipeDirection: "right" | "left", toValueY?: number) => {
const sign = swipeDirection === "left" ? -1 : 1
// Move the card off the screen
const handleLeftSwipe = (toValueY?: number) => {
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 refactor duplicates the handler method for left and right swipes, and in return, I think it makes the component easier to read and extend in the future.

swipeRight()
await waitFor(() => expect(mockOnSwipeRight).toHaveBeenCalledOnce())
})
})
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 wanted to add a test to assert that omitting the onSwipeLeft and onSwipeRight handler does not cause the top card to be removed when a user performs that action. However, I realized that FancySwiper is not responsible for removing the top card, so that test belongs to the parent components that use this component.

@iskounen iskounen changed the title feat(DIA-1015): allow fancy swipe to prevent right swiping feat(DIA-1015): allow FancySwipe to prevent left and/or right swiping Jan 9, 2025
Copy link
Member

@MounirDhahri MounirDhahri left a comment

Choose a reason for hiding this comment

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

Impressive work and nice UX. I like the bounciness there 💟

duration: 300,
useNativeDriver: true,
}).start(() => {
removeCardFromTop(swipeDirection)
// revert the pan responder to its initial position
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I see you are not using the friction as in from line 42 to 46.
Animated.spring(swiper, { toValue: { x: 0, y: 0 }, friction: 5, useNativeDriver: true, }).start()
It might a nice idea to consolidate both into one method

Copy link
Member

Choose a reason for hiding this comment

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

also line 75

Copy link
Member

Choose a reason for hiding this comment

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

impressive

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