-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
explicitly show how to add the optional listener #15576
Conversation
The previous example only showed where to add the optional "listener" but didn't show how to make use of it.
@facebook-github-bot label Core Team Generated by 🚫 dangerJS |
@@ -2857,7 +2857,7 @@ module.exports = { | |||
*```javascript | |||
* onScroll={Animated.event( | |||
* [{nativeEvent: {contentOffset: {x: this._scrollX}}}], | |||
* {listener}, // Optional async listener | |||
* {listener: (event, gestureState) => console.log(gestureState)}, // Optional async listener |
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.
Since this looks like an onScroll event from ScrollView I think we should not add the gestureState param, might be more confusing than anything else. Maybe just (event) => console.log(event)
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.
I trust your opinion, but gestureState is passed along as the second param... so isn't it better to document 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.
Even for ScrollView onScroll? I thought it was only for PanResponder events.
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 point, maybe I should move the example to onPanResponderMove... or add another?
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.
Sure lets just change to onPanResponderMove instead
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.
Ok, done! I left the example for onScroll too. Can only help, right?
@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
The previous example only showed where to add the optional "listener" but didn't show how to make use of it.
(Write your motivation here.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work. Bonus points for screenshots and videos!)