allow custom ripple radius on TouchableNativeFeedback #28009

vonovak · 2020-02-09T20:55:18Z

Summary

motivation: there are cases where one'd like to control the radius of the ripple effect that's present on TouchableNativeFeedback - in my case, I want to make sure that both icons and text have the same ripple appearance, but that's currently not possible as far as I can tell.

Currently (afaik) the only way to set (upper) ripple limits is by specifying width, height and border radius ( + overflow: hidden), and this works well for icons which can usually be bounded by a square, but not for text which can have rectangular shape.

This PR adds rippleRadius parameter to SelectableBackground(), SelectableBackgroundBorderless() and Ripple() static functions present on TouchableNativeFeedback. It can make the ripple smaller but also larger. The result looks like this:

added to RNTester:

difference from the other ripples:

I'm ofc open to changing the api if needed, but I'm not sure there's much space for manoeuvring. While I was at it, I did a slight refactor of the class into several smaller, more focused methods.

It's possible that in some cases, this might help to work around this issue #6480.

Changelog

[Android] [Added] - allow setting custom ripple radius on TouchableNativeFeedback

Test Plan

I tested this locally using RNTester

dulmandakh

LGTM

mdvacca · 2020-02-14T19:26:47Z

CC @TheSavior

elicwhite · 2020-02-14T21:20:23Z

The ripples that go inwards instead of outwards seem weird to me, is that intended?

Also, can you add examples to RNTester that demonstrate this behavior?

vonovak · 2020-02-14T22:29:32Z

@TheSavior regarding the ripple going inwards - that indeed surprised me too, but that seems to be the way it works by default in android and I didn't find a way to change that docs.

If you look at the gif, you'll see that the ripple effect on the second to last touchable already starts pretty big, because the touchable is large and the ripple is borderless. In the last item, it also starts pretty large and only then it "goes toward" the specified radius, which in that case means making the ripple smaller.

I'm guessing that the underlying implementation determines the size that ripple has upon touch (which is given by the size of the target, ignoring the radius), and then it drives the ripple toward the set radius. It does not care if that means making the ripple smaller or bigger ¯_(ツ)_/¯.

I've added 3 examples to RNTester

analysis-bot · 2020-02-14T22:46:44Z

RNTester (Android/hermes/arm64-v8a): 3289088 bytes
RNTester (Android/hermes/armeabi-v7a): 3129344 bytes
RNTester (Android/hermes/x86): 3461120 bytes
RNTester (Android/hermes/x86_64): 3424256 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4644864 bytes

analysis-bot · 2020-02-14T22:51:04Z

RNTester.app (iOS): 10657792 bytes

elicwhite · 2020-02-15T01:11:15Z

Thanks! Can you update your PR summary with a video of the new RNTester example? That way we can look back at this PR to remember how it was supposed to work when it is refactored in the future. :)

vonovak · 2020-02-19T17:26:51Z

@TheSavior added the RNTester gif to the description

analysis-bot · 2020-02-19T17:43:18Z

RNTester (Android/hermes/arm64-v8a): 3291136 bytes
RNTester (Android/hermes/armeabi-v7a): 3129344 bytes
RNTester (Android/hermes/x86): 3461120 bytes
RNTester (Android/hermes/x86_64): 3424256 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4644864 bytes

analysis-bot · 2020-02-19T17:46:33Z

RNTester.app (iOS): 10657792 bytes

elicwhite · 2020-02-19T19:21:14Z

Thanks! The JS changes seem fine. I defer the review to @mdvacca for the Android changes

mdvacca

@vonovak thanks for working on this, aI added a couple of minor comments

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactDrawableHelper.java

analysis-bot · 2020-02-20T08:29:54Z

RNTester (Android/hermes/arm64-v8a): 3289088 bytes
RNTester (Android/hermes/armeabi-v7a): 3129344 bytes
RNTester (Android/hermes/x86): 3461120 bytes
RNTester (Android/hermes/x86_64): 3424256 bytes
RNTester (Android/jsc/arm64-v8a): 4446208 bytes
RNTester (Android/jsc/armeabi-v7a): 4268032 bytes
RNTester (Android/jsc/x86): 4358144 bytes
RNTester (Android/jsc/x86_64): 4644864 bytes

analysis-bot · 2020-02-20T08:32:45Z

RNTester.app (iOS): 10657792 bytes

mdvacca

Looks good to me!

facebook-github-bot

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

react-native-bot · 2020-02-20T23:30:06Z

This pull request was successfully merged by @vonovak in 7f2a79f.

^{When will my fix make it into a release? | Upcoming Releases}

elicwhite · 2020-02-21T04:13:43Z

Hey @vonovak! We just landed a new Pressable component that has very similar code to TouchableNativeFeedback. We didn’t make this same change to it before landing it, would you be willing to make that change? 3212f7d

vonovak · 2020-02-21T08:19:02Z

@TheSavior sure thing!

Summary: motivation: there are cases where one'd like to control the radius of the ripple effect that's present on TouchableNativeFeedback - in my case, I want to make sure that both icons and text have the same ripple appearance, but that's currently not possible as far as I can tell. Currently (afaik) the only way to set (upper) ripple limits is by specifying width, height and border radius ( + `overflow: hidden`), and this works well for icons which can usually be bounded by a square, but not for text which can have rectangular shape. This PR adds `rippleRadius` parameter to `SelectableBackground()`, `SelectableBackgroundBorderless()` and `Ripple()` static functions present on `TouchableNativeFeedback`. It can make the ripple smaller but also larger. The result looks like this: added to RNTester: ![SVID_20200219_182027_1](https://user-images.githubusercontent.com/1566403/74858131-147ff380-5345-11ea-8a9e-2730b79eec38.gif) difference from the other ripples: ![SVID_20200209_110918_1](https://user-images.githubusercontent.com/1566403/74109152-4513a080-4b81-11ea-8ec3-bb5862c57244.gif) I'm ofc open to changing the api if needed, but I'm not sure there's much space for manoeuvring. While I was at it, I did a slight refactor of the class into several smaller, more focused methods. It's possible that in some cases, this might help to work around this issue facebook#6480. ## Changelog [Android] [Added] - allow setting custom ripple radius on TouchableNativeFeedback Pull Request resolved: facebook#28009 Test Plan: I tested this locally using RNTester Reviewed By: TheSavior Differential Revision: D20004509 Pulled By: mdvacca fbshipit-source-id: 10de1754d54c17878f36a3859705c1188f15c2a2

Summary: Motivation is to support ripple radius just like in TouchableNativeFeedback, plus borderless attribute. See #28009 (comment) In the current form this means user needs to pass an `android_ripple` prop which is an object of this shape: ``` export type RippleConfig = {| color?: ?ColorValue, borderless?: ?boolean, radius?: ?number, |}; ``` Do we want to add methods that would create such config objects - https://facebook.github.io/react-native/docs/touchablenativefeedback#methods ? ## Changelog [Android] [Added] - support borderless and custom ripple radius on Pressable Pull Request resolved: #28156 Test Plan: Tested locally in RNTester. I noticed that when some content is rendered after the touchables, the ripple effect is "cut off" by the boundaries of the next view. This is not specific to Pressable, it happens to TouchableNativeFeedback too but I just didn't notice it before in #28009. As it is an issue of its own, I didn't investigate that. ![pressable](https://user-images.githubusercontent.com/1566403/75098762-785f2200-55ba-11ea-8842-e648317610e3.gif) I changed the Touchable example slightly too (I just moved the "custom ripple radius" up to show the "cutting off" issue), so just for completeness: ![touchable](https://user-images.githubusercontent.com/1566403/75098763-81e88a00-55ba-11ea-9528-e0343d1e054b.gif) Reviewed By: yungsters Differential Revision: D20071021 Pulled By: TheSavior fbshipit-source-id: cb553030934205a52dd50a2a8c8a20da6100e23f

Summary: Motivation is to support ripple radius just like in TouchableNativeFeedback, plus borderless attribute. See facebook#28009 (comment) In the current form this means user needs to pass an `android_ripple` prop which is an object of this shape: ``` export type RippleConfig = {| color?: ?ColorValue, borderless?: ?boolean, radius?: ?number, |}; ``` Do we want to add methods that would create such config objects - https://facebook.github.io/react-native/docs/touchablenativefeedback#methods ? ## Changelog [Android] [Added] - support borderless and custom ripple radius on Pressable Pull Request resolved: facebook#28156 Test Plan: Tested locally in RNTester. I noticed that when some content is rendered after the touchables, the ripple effect is "cut off" by the boundaries of the next view. This is not specific to Pressable, it happens to TouchableNativeFeedback too but I just didn't notice it before in facebook#28009. As it is an issue of its own, I didn't investigate that. ![pressable](https://user-images.githubusercontent.com/1566403/75098762-785f2200-55ba-11ea-8842-e648317610e3.gif) I changed the Touchable example slightly too (I just moved the "custom ripple radius" up to show the "cutting off" issue), so just for completeness: ![touchable](https://user-images.githubusercontent.com/1566403/75098763-81e88a00-55ba-11ea-9528-e0343d1e054b.gif) Reviewed By: yungsters Differential Revision: D20071021 Pulled By: TheSavior fbshipit-source-id: cb553030934205a52dd50a2a8c8a20da6100e23f

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 9, 2020

vonovak changed the title ~~allow setting custom ripple radius~~ allow custom ripple radius on TouchableNativeFeedback Feb 9, 2020

react-native-bot added Platform: Android Android applications. Type: Enhancement A new feature or enhancement of an existing feature. labels Feb 9, 2020

dulmandakh approved these changes Feb 10, 2020

View reviewed changes

dulmandakh requested a review from mdvacca February 10, 2020 09:59

vonovak force-pushed the enableCustomRippleRadius branch from ac2cb89 to 7cd895d Compare February 14, 2020 22:27

vonovak force-pushed the enableCustomRippleRadius branch from 7cd895d to 70a706a Compare February 19, 2020 17:23

vonovak mentioned this pull request Feb 19, 2020

Report size of app bundles on PRs (#28019) #28041

Closed

mdvacca reviewed Feb 19, 2020

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactDrawableHelper.java Outdated Show resolved Hide resolved

ReactAndroid/src/main/java/com/facebook/react/views/view/ReactDrawableHelper.java Outdated Show resolved Hide resolved

vonovak added 5 commits February 20, 2020 09:10

allow setting custom ripple radius

1c7dfe5

fix flow

8e8ae5c

add ripple radius example

ed8701b

add non-borderless ripple

e99b6c2

address review

0c74bf7

vonovak force-pushed the enableCustomRippleRadius branch from 78e6264 to 0c74bf7 Compare February 20, 2020 08:10

vonovak requested review from mdvacca and dulmandakh February 20, 2020 08:11

mdvacca approved these changes Feb 20, 2020

View reviewed changes

facebook-github-bot reviewed Feb 20, 2020

View reviewed changes

facebook-github-bot closed this in 7f2a79f Feb 20, 2020

react-native-bot added the Merged This PR has been merged. label Feb 20, 2020

vonovak deleted the enableCustomRippleRadius branch February 21, 2020 08:32

vonovak mentioned this pull request Feb 22, 2020

add ripple config object to Pressable #28156

Closed

vonovak mentioned this pull request May 19, 2020

implement rippleRadius for TouchableNativeFeedback software-mansion/react-native-gesture-handler#1069

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow custom ripple radius on TouchableNativeFeedback #28009

allow custom ripple radius on TouchableNativeFeedback #28009

vonovak commented Feb 9, 2020 •

edited

Loading

dulmandakh left a comment

mdvacca commented Feb 14, 2020

elicwhite commented Feb 14, 2020

vonovak commented Feb 14, 2020 •

edited

Loading

analysis-bot commented Feb 14, 2020

analysis-bot commented Feb 14, 2020

elicwhite commented Feb 15, 2020

vonovak commented Feb 19, 2020

analysis-bot commented Feb 19, 2020

analysis-bot commented Feb 19, 2020

elicwhite commented Feb 19, 2020

mdvacca left a comment

analysis-bot commented Feb 20, 2020

analysis-bot commented Feb 20, 2020

mdvacca left a comment

facebook-github-bot left a comment

react-native-bot commented Feb 20, 2020

elicwhite commented Feb 21, 2020

vonovak commented Feb 21, 2020

allow custom ripple radius on TouchableNativeFeedback #28009

allow custom ripple radius on TouchableNativeFeedback #28009

Conversation

vonovak commented Feb 9, 2020 • edited Loading

Summary

Changelog

Test Plan

dulmandakh left a comment

Choose a reason for hiding this comment

mdvacca commented Feb 14, 2020

elicwhite commented Feb 14, 2020

vonovak commented Feb 14, 2020 • edited Loading

analysis-bot commented Feb 14, 2020

analysis-bot commented Feb 14, 2020

elicwhite commented Feb 15, 2020

vonovak commented Feb 19, 2020

analysis-bot commented Feb 19, 2020

analysis-bot commented Feb 19, 2020

elicwhite commented Feb 19, 2020

mdvacca left a comment

Choose a reason for hiding this comment

analysis-bot commented Feb 20, 2020

analysis-bot commented Feb 20, 2020

mdvacca left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Feb 20, 2020

elicwhite commented Feb 21, 2020

vonovak commented Feb 21, 2020

vonovak commented Feb 9, 2020 •

edited

Loading

vonovak commented Feb 14, 2020 •

edited

Loading