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.

Sign up for GitHub

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

Jump to bottom

Fix TouchNativeFeedback so that the ripple starts from where a person touches #5400

Closed
wants to merge 1 commit into from
Closed

Fix TouchNativeFeedback so that the ripple starts from where a person touches #5400

wants to merge 1 commit into from

Conversation

TheBerg
Copy link
Contributor

@TheBerg TheBerg commented Jan 19, 2016

It needs the touch coordinates for with-in the element, not for on the page.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @sahrens, @mkonicek and @jmstout to be potential reviewers.

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

ide commented Jan 19, 2016

This is way better... @mkonicek could you cc the best person to look at this?

@satya164
Copy link
Contributor

+1

@mkonicek
Copy link
Contributor

cc @dmmiller, @kmagiera

@mkonicek
Copy link
Contributor

lgtm! The change in Touchable.js only affects TouchableNativeFeedback and even if it were used in other views relative coordinates are likely what we want.

I've just tried in the Ads Manager and the ripples do seem to start at the correct positions though, which is weird? Can you show a screen recording before / after?

@dmmiller
Copy link

This seems right. I second the screen recording to verify, but otherwise Thanks!

@TheBerg
Copy link
Contributor Author

TheBerg commented Jan 21, 2016

Here is a before:
before
and after:
after

Let me know if you need anything else.

@ide
Copy link
Contributor

ide commented Jan 21, 2016

Thanks!

@ide
Copy link
Contributor

ide commented Jan 21, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1564464723844674/int_phab to review.

@TheBerg
Copy link
Contributor Author

TheBerg commented Jan 21, 2016

Shoot, wait, while I was just playing around with this I found this:
still
It seems like it is giving the coordinates of a sub element. :-/

first click: 104.23681640625, 7.927642822265625
second click: 27.563364028930664 7.586151123046875

This PR is still better than what is in the repo, but is ultimately not the right answer. Any ideas anyone?

@ghost ghost closed this in 48117ce Jan 21, 2016
MattFoley pushed a commit to skillz/react-native that referenced this pull request Jan 21, 2016
Fix TouchNativeFeedback so that the ripple starts from where a person…
b291b6c 
… touches

Summary:
It needs the touch coordinates for with-in the element, not for on the page.
Closes facebook#5400

Reviewed By: svcscm

Differential Revision: D2848834

Pulled By: androidtrunkagent

fb-gh-sync-id: 88cf0fd7bd2332eb3db835b26438064412c8358c
doostin pushed a commit to doostin/react-native that referenced this pull request Feb 1, 2016
@doostin
Fix TouchNativeFeedback so that the ripple starts from where a person…
817bd89 
… touches

Summary:
It needs the touch coordinates for with-in the element, not for on the page.
Closes facebook#5400

Reviewed By: svcscm

Differential Revision: D2848834

Pulled By: androidtrunkagent

fb-gh-sync-id: 88cf0fd7bd2332eb3db835b26438064412c8358c
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@TheBerg @facebook-github-bot @ide @satya164 @mkonicek @dmmiller