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 3dTouch props to touch events #3055

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
@mattapperson
Contributor

mattapperson commented Sep 26, 2015

This adds the first of the three 3dTouch API types, that found on the touch event.

It adds the following props to touch events when running on iOS 9 devices:

  • touch
  • maxTouch
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 26, 2015

Collaborator

cc @nicklockwood @jordwalke

This looks right to me.

Collaborator

ide commented Sep 26, 2015

cc @nicklockwood @jordwalke

This looks right to me.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Sep 26, 2015

Contributor

CI fail does not seem to be connected to this PR directly...

Contributor

mattapperson commented Sep 26, 2015

CI fail does not seem to be connected to this PR directly...

Show outdated Hide outdated React/Base/RCTTouchHandler.m
@@ -150,6 +150,11 @@ - (void)_updateReactTouchAtIndex:(NSInteger)touchIndex
reactTouch[@"locationX"] = @(touchViewLocation.x);
reactTouch[@"locationY"] = @(touchViewLocation.y);
reactTouch[@"timestamp"] = @(nativeTouch.timestamp * 1000); // in ms, for JS
if([UIDevice currentDevice].systemVersion.floatValue >= 9) {

This comment has been minimized.

@vjeux

vjeux Sep 26, 2015

Contributor

nit: space before (

@vjeux

vjeux Sep 26, 2015

Contributor

nit: space before (

Show outdated Hide outdated React/Base/RCTTouchHandler.m
if([UIDevice currentDevice].systemVersion.floatValue >= 9) {
reactTouch[@"force"] = @(nativeTouch.force);
reactTouch[@"maxForce"] = @(nativeTouch.maximumPossibleForce);

This comment has been minimized.

@vjeux

vjeux Sep 26, 2015

Contributor

Can you use the same name as the native API? The only reason to deviate would be to follow an existing web specification but I don't think there's any yet :)

@vjeux

vjeux Sep 26, 2015

Contributor

Can you use the same name as the native API? The only reason to deviate would be to follow an existing web specification but I don't think there's any yet :)

This comment has been minimized.

@kmagiera

kmagiera Sep 28, 2015

Contributor

I'd rather avoid another iOSisms in the attribute naming, those two properties can and should be cross-platform on android referring to MotionEvent#getPressure (max is fixed to 1.0)

@kmagiera

kmagiera Sep 28, 2015

Contributor

I'd rather avoid another iOSisms in the attribute naming, those two properties can and should be cross-platform on android referring to MotionEvent#getPressure (max is fixed to 1.0)

This comment has been minimized.

@mattapperson

mattapperson Sep 28, 2015

Contributor

@kmagiera I agree. Hence force/maxForce (it's going to be "like" one or the other but not being exactly like iOS keeps thing on level ground)

@mattapperson

mattapperson Sep 28, 2015

Contributor

@kmagiera I agree. Hence force/maxForce (it's going to be "like" one or the other but not being exactly like iOS keeps thing on level ground)

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Sep 26, 2015

Contributor

Sweet, super easy!

Contributor

vjeux commented Sep 26, 2015

Sweet, super easy!

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Sep 26, 2015

Contributor

@vjeux yea :) but I do have a question, I would like to see the supports3dtouch prop merged with this, but not sure the best place to put that API. Any recommendations?

Contributor

mattapperson commented Sep 26, 2015

@vjeux yea :) but I do have a question, I would like to see the supports3dtouch prop merged with this, but not sure the best place to put that API. Any recommendations?

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Sep 26, 2015

Collaborator

But my initial reaction is those Web APIs aren't optimal and it's better for the RN gesture system to include the force info with the touches.

Collaborator

ide commented Sep 26, 2015

But my initial reaction is those Web APIs aren't optimal and it's better for the RN gesture system to include the force info with the touches.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Sep 26, 2015

Contributor

@ide 100% agree

Contributor

mattapperson commented Sep 26, 2015

@ide 100% agree

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Sep 27, 2015

Contributor

I would like to see the supports3dtouch prop merged with this

Would be better to do it in two steps. This pull request for the easy part that we can merge right now. And for the props that need more thoughts, another one.

Contributor

vjeux commented Sep 27, 2015

I would like to see the supports3dtouch prop merged with this

Would be better to do it in two steps. This pull request for the easy part that we can merge right now. And for the props that need more thoughts, another one.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 13, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 13, 2015

Contributor

@vjeux Sorry for the delay, been too busy working with RN to work on it :(

I fixed the code style issue, but the API I would like more discussion on due to Android supporting force touch too and the web API for force-touch being ugly/nasty...

Contributor

mattapperson commented Oct 13, 2015

@vjeux Sorry for the delay, been too busy working with RN to work on it :(

I fixed the code style issue, but the API I would like more discussion on due to Android supporting force touch too and the web API for force-touch being ugly/nasty...

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 13, 2015

Collaborator

If the web API is deficient I think it makes a lot more sense to pick a consistent API across iOS & Android and let RN flourish. Seems like the main question is do we use "force" or "pressure"? Or both because they are actually different concepts?

Collaborator

ide commented Oct 13, 2015

If the web API is deficient I think it makes a lot more sense to pick a consistent API across iOS & Android and let RN flourish. Seems like the main question is do we use "force" or "pressure"? Or both because they are actually different concepts?

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 13, 2015

Contributor

@ide More or less the same. I say force as iOS is the primary and most constant API.
The bigger question is... some systems have a hard max (1.0) so "maxTouch" does not get returned as 1.0 is "known" to be max. iOS native mass is passed as an API as the value can and is over 1.0 at times.

My argument is to use iOS api and the other platforms would just always return 1.0 as max. This is the easiest code to maintain in core.

Alternatively we could divide the touch value for iOS so that iOS too always has 1.0 as max (thus no need to return max in the API)

Contributor

mattapperson commented Oct 13, 2015

@ide More or less the same. I say force as iOS is the primary and most constant API.
The bigger question is... some systems have a hard max (1.0) so "maxTouch" does not get returned as 1.0 is "known" to be max. iOS native mass is passed as an API as the value can and is over 1.0 at times.

My argument is to use iOS api and the other platforms would just always return 1.0 as max. This is the easiest code to maintain in core.

Alternatively we could divide the touch value for iOS so that iOS too always has 1.0 as max (thus no need to return max in the API)

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 13, 2015

Collaborator

My argument is to use iOS api and the other platforms would just always return 1.0 as max. This is the easiest code to maintain in core.

this sounds reasonable. I don't think we should normalize to 1.0 right now, or if we do that's the kind of thing that should happen in JS.

Collaborator

ide commented Oct 13, 2015

My argument is to use iOS api and the other platforms would just always return 1.0 as max. This is the easiest code to maintain in core.

this sounds reasonable. I don't think we should normalize to 1.0 right now, or if we do that's the kind of thing that should happen in JS.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 13, 2015

Contributor

@ide so we are good to go then?

Contributor

mattapperson commented Oct 13, 2015

@ide so we are good to go then?

Show outdated Hide outdated React/Base/RCTTouchHandler.m
@@ -150,6 +150,11 @@ - (void)_updateReactTouchAtIndex:(NSInteger)touchIndex
reactTouch[@"locationX"] = @(touchViewLocation.x);
reactTouch[@"locationY"] = @(touchViewLocation.y);
reactTouch[@"timestamp"] = @(nativeTouch.timestamp * 1000); // in ms, for JS
if ([UIDevice currentDevice].systemVersion.floatValue >= 9) {

This comment has been minimized.

@ide

ide Oct 13, 2015

Collaborator

This check should be more dynamic. Check if forceTouchCapability is defined (iOS 9+) and enabled.

@ide

ide Oct 13, 2015

Collaborator

This check should be more dynamic. Check if forceTouchCapability is defined (iOS 9+) and enabled.

This comment has been minimized.

@mattapperson

mattapperson Oct 13, 2015

Contributor

I agree that check needs to exist... but I think dev should do that in-app (using another API). as 99% of the time ( i assume) dev would need to also alter or disable the feature.
As it is written, the values are automatically null if the user has iOS 9 but no force touch / not enabled

@mattapperson

mattapperson Oct 13, 2015

Contributor

I agree that check needs to exist... but I think dev should do that in-app (using another API). as 99% of the time ( i assume) dev would need to also alter or disable the feature.
As it is written, the values are automatically null if the user has iOS 9 but no force touch / not enabled

This comment has been minimized.

@ide

ide Oct 13, 2015

Collaborator

Ok can you just replace the iOS 9 check with whether forceTouchCapability is defined by the system OS (regardless of its value?)

@ide

ide Oct 13, 2015

Collaborator

Ok can you just replace the iOS 9 check with whether forceTouchCapability is defined by the system OS (regardless of its value?)

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 14, 2015

Contributor

OK updated to check for traitCollection and traitCollection.forceTouchCapability

Contributor

mattapperson commented Oct 14, 2015

OK updated to check for traitCollection and traitCollection.forceTouchCapability

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

Show outdated Hide outdated React/Base/RCTTouchHandler.m
@@ -150,6 +150,13 @@ - (void)_updateReactTouchAtIndex:(NSInteger)touchIndex
reactTouch[@"locationX"] = @(touchViewLocation.x);
reactTouch[@"locationY"] = @(touchViewLocation.y);
reactTouch[@"timestamp"] = @(nativeTouch.timestamp * 1000); // in ms, for JS
if ([nativeTouch.view respondsToSelector:@selector(traitCollection)] &&
[[[nativeTouch view] traitCollection] respondsToSelector:@selector(forceTouchCapability)] {

This comment has been minimized.

@ide

ide Oct 14, 2015

Collaborator

nit: use property syntax instead of calls: nativeTouch.view.traitCollection

@ide

ide Oct 14, 2015

Collaborator

nit: use property syntax instead of calls: nativeTouch.view.traitCollection

This comment has been minimized.

@mattapperson

mattapperson Oct 14, 2015

Contributor

heh I noticed that style and changed it 1 line up... my bad

@mattapperson

mattapperson Oct 14, 2015

Contributor

heh I noticed that style and changed it 1 line up... my bad

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 14, 2015

Collaborator

LGTM -- @nicklockwood could you take a look?

Collaborator

ide commented Oct 14, 2015

LGTM -- @nicklockwood could you take a look?

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 19, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 19, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@mkonicek

This comment has been minimized.

Show comment
Hide comment
@mkonicek

mkonicek Oct 24, 2015

Contributor

Looks OK based on the discussion above. @javache Do you think we should merge this? Not sure if it could affect perf - is the code called on every move event?

Contributor

mkonicek commented Oct 24, 2015

Looks OK based on the discussion above. @javache Do you think we should merge this? Not sure if it could affect perf - is the code called on every move event?

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Oct 24, 2015

Member

Great question @mkonicek : Would it be possible to just do the check once at load time (on a window view or something)?

Member

jordwalke commented Oct 24, 2015

Great question @mkonicek : Would it be possible to just do the check once at load time (on a window view or something)?

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 24, 2015

Contributor

No perf hit for the check as far as I can tell...

Contributor

mattapperson commented Oct 24, 2015

No perf hit for the check as far as I can tell...

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Oct 24, 2015

Member

I may not be understanding how iOS 3D touch support works, but it's pretty standard practice to perform feature detection once at startup, instead of every time an event occurs. I'd suggest that slight improvement to this PR.

Member

jordwalke commented Oct 24, 2015

I may not be understanding how iOS 3D touch support works, but it's pretty standard practice to perform feature detection once at startup, instead of every time an event occurs. I'd suggest that slight improvement to this PR.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 24, 2015

Contributor

@jordwalke and where should the value of that check be held?

Contributor

mattapperson commented Oct 24, 2015

@jordwalke and where should the value of that check be held?

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Oct 24, 2015

Member

It could even live right where it is (if you can't find a better place) but you can store the result in a static variable that is only ever set once.

I might be misunderstanding force touch (apologies if so). Thanks for adding this btw!

Member

jordwalke commented Oct 24, 2015

It could even live right where it is (if you can't find a better place) but you can store the result in a static variable that is only ever set once.

I might be misunderstanding force touch (apologies if so). Thanks for adding this btw!

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 24, 2015

Contributor

@jordwalke I think the confusion comes from the fact that I was asked to do this in this way. I orig had just a check for if the value existed, pass it on. I was then asked to use this API.
in my mind thats not ideal, it should be left its own API. I offered to do that API as its own and was told to do that in a separate PR.
More then happy to make the change... but feeling a little whiplashed from this small PR. :)

Contributor

mattapperson commented Oct 24, 2015

@jordwalke I think the confusion comes from the fact that I was asked to do this in this way. I orig had just a check for if the value existed, pass it on. I was then asked to use this API.
in my mind thats not ideal, it should be left its own API. I offered to do that API as its own and was told to do that in a separate PR.
More then happy to make the change... but feeling a little whiplashed from this small PR. :)

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 24, 2015

Contributor

To be clear... happy to make the change... just explaining my frustration over a 7 line PR :)

Contributor

mattapperson commented Oct 24, 2015

To be clear... happy to make the change... just explaining my frustration over a 7 line PR :)

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 24, 2015

Collaborator

@jordwalke the reason for detecting features each time is that users can disable force touch (according to the apple docs anyway). Also multiscreen apps but apple doesn't do that yet.

Collaborator

ide commented Oct 24, 2015

@jordwalke the reason for detecting features each time is that users can disable force touch (according to the apple docs anyway). Also multiscreen apps but apple doesn't do that yet.

@jordwalke

This comment has been minimized.

Show comment
Hide comment
@jordwalke

jordwalke Oct 24, 2015

Member

Then that is a perfectly valid reason to do the check every time! I was just trying (poorly) to be helpful since the diff had been sitting without attention. Carry on people!

Member

jordwalke commented Oct 24, 2015

Then that is a perfectly valid reason to do the check every time! I was just trying (poorly) to be helpful since the diff had been sitting without attention. Carry on people!

@ide

This comment has been minimized.

Show comment
Hide comment
@ide

ide Oct 25, 2015

Collaborator

@mattapperson thanks for bearing with us. A PR that affects every touch event is critical enough to RN that we want to be extra deliberate about considering the API, perf, and how this will look on other platforms, compared to a 1000-line PR adding demos to the UIExplorer. That said this PR looks good to me, possibly with some trivial name changes to the properties, though they seem fine to me.

So - this is the current plan:
iOS will send force and maxForce from the native layer with each touch event. This will enable some experiments with 3D Touch, notably if we need to make changes to the JS gesture system.

Later:
If maxForce is not 1.0, we may want to normalize the force in JS. Not sure we want to do this though, so will document that this is subject to change.
Look into exposing the angle of the touch on the iPad Pro

Collaborator

ide commented Oct 25, 2015

@mattapperson thanks for bearing with us. A PR that affects every touch event is critical enough to RN that we want to be extra deliberate about considering the API, perf, and how this will look on other platforms, compared to a 1000-line PR adding demos to the UIExplorer. That said this PR looks good to me, possibly with some trivial name changes to the properties, though they seem fine to me.

So - this is the current plan:
iOS will send force and maxForce from the native layer with each touch event. This will enable some experiments with 3D Touch, notably if we need to make changes to the JS gesture system.

Later:
If maxForce is not 1.0, we may want to normalize the force in JS. Not sure we want to do this though, so will document that this is subject to change.
Look into exposing the angle of the touch on the iPad Pro

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Oct 25, 2015

Contributor

@ide it's cool, I understand. I'm coming from the titanium camp. Was rather entrenched there... This is still better then that, so all is good :)

Contributor

mattapperson commented Oct 25, 2015

@ide it's cool, I understand. I'm coming from the titanium camp. Was rather entrenched there... This is still better then that, so all is good :)

@@ -150,6 +150,13 @@ - (void)_updateReactTouchAtIndex:(NSInteger)touchIndex
reactTouch[@"locationX"] = @(touchViewLocation.x);
reactTouch[@"locationY"] = @(touchViewLocation.y);
reactTouch[@"timestamp"] = @(nativeTouch.timestamp * 1000); // in ms, for JS
if ([nativeTouch.view respondsToSelector:@selector(traitCollection)] &&
[nativeTouch.view.traitCollection respondsToSelector:@selector(forceTouchCapability)] {

This comment has been minimized.

@ptmt

ptmt Oct 29, 2015

Contributor

Missing closing parenthesis here.

@ptmt

ptmt Oct 29, 2015

Contributor

Missing closing parenthesis here.

This comment has been minimized.

@2upmedia

2upmedia Dec 29, 2015

^ That's why travis is failing

@2upmedia

2upmedia Dec 29, 2015

^ That's why travis is failing

This comment has been minimized.

@mattapperson

mattapperson Dec 29, 2015

Contributor

@2upmedia yes. As stated in the comments I am waiting on a final API decision to update the PR

@mattapperson

mattapperson Dec 29, 2015

Contributor

@2upmedia yes. As stated in the comments I am waiting on a final API decision to update the PR

@mkonicek mkonicek added the 🔷iOS label Oct 29, 2015

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Nov 2, 2015

Contributor

http://ashertrockman.github.io/ios/2015/10/24/3d-touch-scale.html

Safari actually implements this by having a field named "force" and normalized from 0 to 1. Would be nice to follow that

Contributor

vjeux commented Nov 2, 2015

http://ashertrockman.github.io/ios/2015/10/24/3d-touch-scale.html

Safari actually implements this by having a field named "force" and normalized from 0 to 1. Would be nice to follow that

@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Dec 12, 2015

Collaborator

This has been sitting dormant for over a month now...just wanted to ping on it. Would be awesome to get this in - including force touch in iOS apps is becoming the standard and would be awesome for RN to support.

/cc @mattapperson @mkonicek

Collaborator

skevy commented Dec 12, 2015

This has been sitting dormant for over a month now...just wanted to ping on it. Would be awesome to get this in - including force touch in iOS apps is becoming the standard and would be awesome for RN to support.

/cc @mattapperson @mkonicek

@skevy skevy added the Size/M label Dec 12, 2015

@marano

This comment has been minimized.

Show comment
Hide comment
@marano

marano Dec 12, 2015

Also would love this see this merged in! @ptmt pointed out here why the CI is broken.

marano commented Dec 12, 2015

Also would love this see this merged in! @ptmt pointed out here why the CI is broken.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Dec 12, 2015

Contributor

There was feedback from @ide and @vjeux on the API - not sure if that's been addressed satisfactorily?

Contributor

nicklockwood commented Dec 12, 2015

There was feedback from @ide and @vjeux on the API - not sure if that's been addressed satisfactorily?

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Dec 13, 2015

Contributor

Yea the moment I get a "final word" on the API I'll finish this up. Same day if I can. Just hard to keep redoing things so I thought I would just wait for concensis

Contributor

mattapperson commented Dec 13, 2015

Yea the moment I get a "final word" on the API I'll finish this up. Same day if I can. Just hard to keep redoing things so I thought I would just wait for concensis

@2upmedia

This comment has been minimized.

Show comment
Hide comment
@2upmedia

2upmedia Dec 23, 2015

Would love to see this merged. Nice work! @mattapperson

Would love to see this merged. Nice work! @mattapperson

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 29, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Dec 29, 2015

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Jan 24, 2016

@mattapperson updated the pull request.

@mattapperson updated the pull request.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 25, 2016

Contributor

OK, I think this has sat around long enough.

I agree with @vjeux, the Safari approach of having force normalized between 0-1 makes the most sense to me. It seems pretty likely that as-and-when other platforms add support for this, they'll follow that standard.

This does mean losing the useful functionality of knowing what the average pressure is, but we can always add an averageForce constant at some point in the future if it seems useful. In the meantime, let's just get this landed.

@mattapperson, if you'd be so kind as to update this PR by removing the maxForce prop and dividing the force by the maximumForce on the native side, I'll accept this.

Contributor

nicklockwood commented Jan 25, 2016

OK, I think this has sat around long enough.

I agree with @vjeux, the Safari approach of having force normalized between 0-1 makes the most sense to me. It seems pretty likely that as-and-when other platforms add support for this, they'll follow that standard.

This does mean losing the useful functionality of knowing what the average pressure is, but we can always add an averageForce constant at some point in the future if it seems useful. In the meantime, let's just get this landed.

@mattapperson, if you'd be so kind as to update this PR by removing the maxForce prop and dividing the force by the maximumForce on the native side, I'll accept this.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Jan 25, 2016

Contributor

@nicklockwood Thanks ill get that in by EOD tomorrow EST!

Contributor

mattapperson commented Jan 25, 2016

@nicklockwood Thanks ill get that in by EOD tomorrow EST!

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 25, 2016

Contributor

@mattapperson if you'd prefer, I can pull this in now and make those changes myself? (you'll still get credit for the PR)

Contributor

nicklockwood commented Jan 25, 2016

@mattapperson if you'd prefer, I can pull this in now and make those changes myself? (you'll still get credit for the PR)

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Jan 25, 2016

Contributor

@nicklockwood makes no difference to me honestly as long as it gets in. I really don't even care so much about the credit. I just have a number of doctors appointments today so I can't until late tonight at the earliest...

Contributor

mattapperson commented Jan 25, 2016

@nicklockwood makes no difference to me honestly as long as it gets in. I really don't even care so much about the credit. I just have a number of doctors appointments today so I can't until late tonight at the earliest...

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
Contributor

nicklockwood commented Jan 25, 2016

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Jan 25, 2016

Collaborator

🚀 🚀 🚀

Collaborator

skevy commented Jan 25, 2016

🚀 🚀 🚀

@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Jan 25, 2016

Collaborator

Thanks @nicklockwood!

Collaborator

skevy commented Jan 25, 2016

Thanks @nicklockwood!

@ghost ghost closed this in fff5dc3 Jan 27, 2016

@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Jan 27, 2016

Collaborator

Wooooooooooo i love when things get merged. :)

Collaborator

skevy commented Jan 27, 2016

Wooooooooooo i love when things get merged. :)

@skevy

This comment has been minimized.

Show comment
Hide comment
@skevy

skevy Jan 27, 2016

Collaborator

Seriously thanks for your diligence on this @mattapperson. I appreciate the patience, and sorry it took us so long!

Collaborator

skevy commented Jan 27, 2016

Seriously thanks for your diligence on this @mattapperson. I appreciate the patience, and sorry it took us so long!

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Jan 27, 2016

Contributor

No worries, just glad it finally got merged

Contributor

mattapperson commented Jan 27, 2016

No worries, just glad it finally got merged

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 27, 2016

Contributor

Four months isn't a bad turnaround I thought ;-)

Seriously though, sorry this took so long. The problem with PRs that add new functionality is that they're often extremely hard to review, especially if we aren't using that functionality ourselves yet, and so can't make an immediate decision about whether the implementation and API is appropriate.

This is an extreme example, being such a tiny PR, but even this generated quite a lot of discussion about the right choice of API, and in the end I wasn't happy to land it until I'd built an example project and verified for myself that the API was usable.

As it happens, I ended up adding a forceTouchAvailable property to View because when I tried to build the example, I immediately realized that the mere presence or absence of a force property in the touch event isn't enough indication to the developer whether the API is available or not, since by the time you've received the touch, it's too late to display alternative input instructions to the user.

So reviewing this seven line PR actually resulted in me spending a couple of days writing a 100-line follow-up commit, which will be landing shortly :-)

I was happy to do that in this case because it's a cool feature, but I hope that yields some insight into why these things sometimes take longer to review than you might expect.

Contributor

nicklockwood commented Jan 27, 2016

Four months isn't a bad turnaround I thought ;-)

Seriously though, sorry this took so long. The problem with PRs that add new functionality is that they're often extremely hard to review, especially if we aren't using that functionality ourselves yet, and so can't make an immediate decision about whether the implementation and API is appropriate.

This is an extreme example, being such a tiny PR, but even this generated quite a lot of discussion about the right choice of API, and in the end I wasn't happy to land it until I'd built an example project and verified for myself that the API was usable.

As it happens, I ended up adding a forceTouchAvailable property to View because when I tried to build the example, I immediately realized that the mere presence or absence of a force property in the touch event isn't enough indication to the developer whether the API is available or not, since by the time you've received the touch, it's too late to display alternative input instructions to the user.

So reviewing this seven line PR actually resulted in me spending a couple of days writing a 100-line follow-up commit, which will be landing shortly :-)

I was happy to do that in this case because it's a cool feature, but I hope that yields some insight into why these things sometimes take longer to review than you might expect.

@mattapperson

This comment has been minimized.

Show comment
Hide comment
@mattapperson

mattapperson Jan 27, 2016

Contributor

@nicklockwood Not to worry I know the plight 😊 I used to work for Appcelerator doing the same for Titanium modules.

Contributor

mattapperson commented Jan 27, 2016

@nicklockwood Not to worry I know the plight 😊 I used to work for Appcelerator doing the same for Titanium modules.

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 27, 2016

Contributor

Here's the follow-up: f685878

Contributor

nicklockwood commented Jan 27, 2016

Here's the follow-up: f685878

doostin added a commit to doostin/react-native that referenced this pull request Feb 1, 2016

Add 3dTouch props to touch events
Summary:
This adds the first of the three 3dTouch API types, that found on the touch event.

It adds the `force` prop to touch events when running on iOS 9 devices:
Closes facebook#3055

Reviewed By: svcscm

Differential Revision: D2860540

Pulled By: nicklockwood

fb-gh-sync-id: 95a3eb433837c844f755b3ed4a3dfcb28452c284

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment