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

Add documentation for onMomentumScrollEnd #15144

Closed
wants to merge 2 commits into from

Conversation

tomasreimers
Copy link
Contributor

No description provided.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jul 21, 2017
Copy link
Contributor

@shergin shergin left a comment

Choose a reason for hiding this comment

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

Could you please clarify in the description the difference with onScrollAnimationEnd?
And please use consistent description style, like "Called when ...".

@tomasreimers
Copy link
Contributor Author

Absolutely! Also, FWIW, I don't think that onScrollAnimationEnd is ever called...

https://snack.expo.io/BJyf5OGIW

RCT_SEND_SCROLL_EVENT(onMomentumScrollEnd, nil); //TODO: shouldn't this be onScrollAnimationEnd?

@facebook-github-bot facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Jul 23, 2017
@facebook-github-bot
Copy link
Contributor

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@shergin
Copy link
Contributor

shergin commented Jul 23, 2017

@tomasreimers Yeah... You are right. Thank you!
I think, we also have to do:

  • Remove onScrollAnimationEnd from documentation and delete all (unused) mentions of it from Android and iOS sources;
  • Implement onMomentumScrollBegin on iOS (it already should work on Android) and add this to documentation.

I will add it to my plans. (Or a PR is always welcome. 😄)

@tomasreimers
Copy link
Contributor Author

tomasreimers commented Jul 23, 2017

@shergin

(1) Done: #15156 (Nothing at FB is someone else's problem, right? ;) )

(2) Already done (I think...):

@shergin
Copy link
Contributor

shergin commented Jul 23, 2017

Awesome! Now we just have to mention this in the documentation.

@tomasreimers
Copy link
Contributor Author

@shergin #15158

facebook-github-bot pushed a commit that referenced this pull request Jul 23, 2017
Summary:
<!--
Thank you for sending the PR!

If you changed any code, please provide us with clear instructions on how you verified your changes work. In other words, a test plan is *required*. Bonus points for screenshots and videos!

Please read the Contribution Guidelines at https://github.com/facebook/react-native/blob/master/CONTRIBUTING.md to learn more about contributing to React Native.

Happy contributing!
-->
Closes #15144

Differential Revision: D5478574

Pulled By: shergin

fbshipit-source-id: 33c49f0efdfb3a518e1ee254b1dc01ec22f09269
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants