Add hitSlop prop on iOS and Android by jesseruder · Pull Request #5720 · react/react-native

jesseruder · 2016-02-03T03:24:49Z

New prop hitSlop allows extending the touch area of Touchable components. This makes it easier to touch small buttons without needing to change your styles.

It takes top, bottom, left, and right same as the pressRetentionOffset prop. When a touch is moved, hitSlop is combined with pressRetentionOffset to determine how far the touch can move off the button before deactivating the button.

On Android I had to add a new file ids.xml to generate a unique ID to use for the tag where I store the hitSlop state. The iOS side is more straightforward.

@terribleben worked on the iOS and JS parts of this diff.

Fixes #110

facebook-github-bot · 2016-02-03T03:25:47Z

By analyzing the blame information on this pull request, we identified @dmmiller, @andreicoman11 and @Ehesp to be potential reviewers.

eslint-bot · 2016-02-03T03:56:42Z

undefined This type is incompatible with number

facebook-github-bot · 2016-02-03T19:50:19Z

@jesseruder updated the pull request.

jesseruder · 2016-02-03T20:36:11Z

It looks like the tests don't like me doing import com.facebook.react.R in TouchTargetHelper. I'm not sure how to avoid this since you need to use a resource file Id or else you get an IllegalArgumentException. See http://stackoverflow.com/questions/2859574/the-key-must-be-an-application-specific-resource-id

ide · 2016-02-03T20:39:08Z

@andreicoman11 @kmagiera are you guys the best people to look at Android touch system diffs?

ide · 2016-02-03T21:09:19Z

@bestander it looks like R.java isn't generated (?) because the tests don't use Gradle -- off the top of your head do you know how to make Buck take care of this for us?

eslint-bot · 2016-02-03T23:25:07Z

undefined This type is incompatible with object type

facebook-github-bot · 2016-02-04T00:06:15Z

@jesseruder updated the pull request.

kmagiera · 2016-02-04T06:45:00Z

Why not just add an extra property to the ReactViewGroup and assing it there? Calling setTag this way will often allocate an extra SparseArray.

Makes sense. I'll change that.

facebook-github-bot · 2016-02-05T01:17:42Z

@jesseruder updated the pull request.

jesseruder · 2016-02-05T02:32:52Z

I tried adding react_native_target('java/com/facebook/react/views/view:view') to ReactAndroid/src/main/java/com/facebook/react/uimanager/BUCK but that gave me:

BUILD FAILED: Cycle found: //ReactAndroid/src/main/java/com/facebook/react/uimanager:uimanager -> //ReactAndroid/src/main/java/com/facebook/react/views/view:view -> //ReactAndroid/src/main/java/com/facebook/react/uimanager:uimanager

Is there any clean way to solve this @bestander? I could add an interface under react/uimanager but that seems like overkill.

kmagiera · 2016-02-05T08:17:10Z

Can you extract getHitSlopRect to an interface so that other views (such as Text elements) can implement hitSlop too if we need it?

bestander · 2016-02-05T10:51:53Z

@jesseruder yeah, that sucks.
Looks like uimanager is quite large but I don't see a quick and clean way to split it.
Possible ways to fix: move TouchTargetHelper.java to view package or into its own package.

@mkonicek @foghina what do you think?

skevy · 2016-02-10T22:03:11Z

@mkonicek iOS side looks 👍 to me. LGTM

skevy · 2016-02-10T22:12:19Z

Also, just FYI martin, we've been using this internally and it's working great on both platforms :)

mkonicek · 2016-02-10T22:16:54Z

Oh cool, good to know!

andreicoman11 · 2016-02-11T11:19:07Z

This should be more obvious to the reader, something like ** NOTE ** or even "Experimental" would help here

facebook-github-bot · 2016-02-11T11:19:13Z

@jesseruder updated the pull request.

andreicoman11 · 2016-02-11T11:19:40Z

same as above

facebook-github-bot · 2016-02-11T14:18:45Z

@jesseruder updated the pull request.

mkonicek · 2016-02-11T17:12:42Z

Can you add a short example here showing how I would extend the area by n pixels in each direction? What's a reasonable number?

facebook-github-bot · 2016-02-11T17:13:14Z

@jesseruder updated the pull request.

mkonicek · 2016-02-11T17:15:18Z

Looks good! Just two small nits and would be nice to add a small example: https://github.com/facebook/react-native/blob/master/Examples/UIExplorer/TouchableExample.js

facebook-github-bot · 2016-02-11T19:19:16Z

@jesseruder updated the pull request.

jesseruder · 2016-02-11T19:19:37Z

@mkonicek @andreicoman11 Added an example and more comments. Thanks!

facebook-github-bot · 2016-02-11T19:22:17Z

@jesseruder updated the pull request.

facebook-github-bot · 2016-02-11T21:35:15Z

@jesseruder updated the pull request.

facebook-github-bot · 2016-02-15T19:40:13Z

@jesseruder updated the pull request.

facebook-github-bot · 2016-02-15T19:43:58Z

@jesseruder updated the pull request.

ide · 2016-02-16T23:26:49Z

The example looks good and this is working on both platforms.

ide · 2016-02-16T23:26:52Z

@facebook-github-bot shipit

facebook-github-bot · 2016-02-16T23:28:27Z

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

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 Feb 3, 2016

eslint-bot reviewed Feb 3, 2016
View reviewed changes

Comment thread Libraries/Components/Touchable/TouchableBounce.js Outdated

eslint-bot Feb 3, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

undefined This type is incompatible with number

jesseruder force-pushed the feature/hit_slop branch from 779f977 to 34cec67 Compare February 3, 2016 19:49

ide added Platform: iOS iOS applications. Android and removed GH Review: review-needed labels Feb 3, 2016

eslint-bot reviewed Feb 3, 2016
View reviewed changes

Comment thread Libraries/Components/Touchable/TouchableWithoutFeedback.js

eslint-bot Feb 3, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

undefined This type is incompatible with object type

jesseruder force-pushed the feature/hit_slop branch from 34cec67 to b5ee125 Compare February 4, 2016 00:05

kmagiera reviewed Feb 4, 2016
View reviewed changes

skevy added GH Review: needs-revision and removed GH Review: review-needed labels Feb 4, 2016

jesseruder force-pushed the feature/hit_slop branch from b5ee125 to a50bc13 Compare February 5, 2016 01:16

kmagiera reviewed Feb 5, 2016
View reviewed changes

jesseruder force-pushed the feature/hit_slop branch from a50bc13 to 6db6921 Compare February 5, 2016 21:22

skevy added GH Review: review-needed and removed GH Review: needs-revision labels Feb 10, 2016

andreicoman11 reviewed Feb 11, 2016
View reviewed changes

Comment thread Libraries/Components/View/View.js Outdated

andreicoman11 Feb 11, 2016

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

same as above

mkonicek reviewed Feb 11, 2016
View reviewed changes

jesseruder force-pushed the feature/hit_slop branch from 6db6921 to 6cdf128 Compare February 11, 2016 19:18

jesseruder force-pushed the feature/hit_slop branch from 6cdf128 to d13c51a Compare February 11, 2016 19:21

jesseruder force-pushed the feature/hit_slop branch from d13c51a to 54dcde6 Compare February 15, 2016 19:39

jesseruder force-pushed the feature/hit_slop branch from 54dcde6 to 8211582 Compare February 15, 2016 19:43

Add hitSlop prop on iOS and Android

951c8bd

jesseruder force-pushed the feature/hit_slop branch from 8211582 to 951c8bd Compare February 15, 2016 19:47

ghost closed this in 0176ac4 Feb 17, 2016

ide deleted the feature/hit_slop branch March 11, 2016 08:41

This pull request was closed.

Conversation

jesseruder commented Feb 3, 2016

Uh oh!

facebook-github-bot commented Feb 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 3, 2016

Uh oh!

jesseruder commented Feb 3, 2016

Uh oh!

ide commented Feb 3, 2016

Uh oh!

ide commented Feb 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 5, 2016

Uh oh!

jesseruder commented Feb 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bestander commented Feb 5, 2016

Uh oh!

skevy commented Feb 10, 2016

Uh oh!

skevy commented Feb 10, 2016

Uh oh!

mkonicek commented Feb 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Feb 11, 2016

Uh oh!

mkonicek commented Feb 11, 2016

Uh oh!

facebook-github-bot commented Feb 11, 2016

Uh oh!

jesseruder commented Feb 11, 2016

Uh oh!

facebook-github-bot commented Feb 11, 2016

Uh oh!

facebook-github-bot commented Feb 11, 2016

Uh oh!

facebook-github-bot commented Feb 15, 2016

Uh oh!

facebook-github-bot commented Feb 15, 2016

Uh oh!

ide commented Feb 16, 2016

Uh oh!

ide commented Feb 16, 2016

Uh oh!

facebook-github-bot commented Feb 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants