Foreground ripple support in Pressable #31632
Conversation
facebook-github-bot
added
the
CLA Signed
Base commit: c774f9e |
|
Looks like the JavaScript tests are failing due to a missing dep in the useMemo array. |
Base commit: c774f9e |
| @@ -180,15 +180,28 @@ function Pressable(props: Props, forwardedRef): React.Node { | |||
| ? {...props.accessibilityState, disabled} | |||
| : props.accessibilityState; | |||
|
|
|||
| const restPropsWithDefaults: React.ElementConfig<typeof View> = { | |||
| let restPropsWithDefaults: React.ElementConfig<typeof View> = { | |||
There was a problem hiding this comment.
Can you keep this as const and instead create a new const local variable that contains the ripple props (or remains undefined) that you spread in here?
It's a minor code reorganization, but it helps keep the code slightly more readable in the long-term the more you can have const variables that — as you read the declaration — you know will not be reassigned later.
| @@ -27,6 +27,7 @@ export type RippleConfig = {| | |||
| color?: ColorValue, | |||
| borderless?: boolean, | |||
| radius?: number, | |||
| useForeground?: boolean, | |||
There was a problem hiding this comment.
Can you just name this foreground? Even though it is a property name and not a variable name, avoiding the "use" prefix is just better in the current world with hooks.
| @@ -58,16 +61,23 @@ export default function useAndroidRippleForView( | |||
| 'Unexpected color given for Ripple color', | |||
| ); | |||
|
|
|||
| let viewProps = {}; | |||
There was a problem hiding this comment.
Similar feedback as above about making this const. You can also instantiating this after backgroundValue:
const viewProps = foreground === true
? {nativeForegroundAndroid: backgroundValue}
: {nativeBackgroundAndroid: backgroundValue};
Could even return this to line 80 as a ternary property initialization in the return statement.
| @@ -98,5 +108,5 @@ export default function useAndroidRippleForView( | |||
| }; | |||
| } | |||
| return null; | |||
| }, [color, borderless, radius, viewRef]); | |||
| }, [color, borderless, radius, viewRef, useForeground]); | |||
There was a problem hiding this comment.
Nit: Alphabetize (after renaming to foreground).
|
@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
@yungsters merged this pull request in 0823f29. |
Summary
This PR aims to enable support for foreground ripple in Pressable. This makes it possible to show ripple on top of custom child components like Image as shown in the below example.
Changelog
[Android] [Added] - Support for foreground ripple in Pressable
Test Plan
Screen.Recording.2021-05-30.at.9.37.48.PM.mov