-
Notifications
You must be signed in to change notification settings - Fork 2
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(EMI-2095): Add Swipeable component #263
Conversation
export interface SwipeableProps { | ||
children: React.ReactNode | ||
actionOnPress: () => void | ||
actionCompoent: React.ReactNode |
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.
actionComponent
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.
Not sure if we should leave it so flexible to be any kind of element, that's why I recommend going through DS first
The code looks fine, and simple to use however I feel like this component should be somehow in DS first, that's the common blackhole of Eigen's UI elements, they exist in product mockups but never in DS, once you search for the source of truth no one can find it. |
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.
Looking good on my side, left only a versioning question
@@ -44,6 +44,7 @@ | |||
"react-native-blurhash": "^1.1.11", | |||
"react-native-collapsible-tab-view": "^8.0.0", | |||
"react-native-fast-image": "^8.6.3", | |||
"react-native-gesture-handler": "2.19.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.
**question: ** Since we are introducing react-native-gesture-handler
in palette why didn't we go for the latest version?
Noticed on the docs that there is this requirements table depending on the react native version that we might need to consult when picking the right version
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.
Good idea! But I have to say that this is PR is not active since we decided to add the Swipeable in Eigen instead (artsy/eigen#10879) 😅 my bad for not keeping track of this one, I'll close it since probably won't add the swipeable to palette anytime soon, but will reopen it once we get back to it :)
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.
ooh okay no worries then
closing this in favor of artsy/eigen#10879 |
This PR resolves EMI-2095
Description
This PR adds a Swipeable component Palette using
react-native-gesture-handler
swipeableDemo
swipeable-android.mov
swipeable-ios.mov