fix: rippleColor shall be declared as integer #3250
Conversation
There was a problem hiding this comment.
Hi, the overall premise of this PR is great, it removes the necessity to use processColor on paper when using hexadecimal colors.
Unfortunately these changes cause a crash when you try setting ripple_color by the color's name (red). I'm currently looking into why fabric works, while paper doesn't, and if the two architectures simply have varying workflows, or some other issues.
| @@ -35,7 +35,7 @@ public void setProperty(T view, String propName, @Nullable Object value) { | |||
| mViewManager.setEnabled(view, value == null ? true : (boolean) value); | |||
| break; | |||
| case "rippleColor": | |||
| mViewManager.setRippleColor(view, ColorPropConverter.getColor(value, view.getContext())); | |||
There was a problem hiding this comment.
Removing ColorPropConverter.getColor(value, view.getContext() breaks paper builds when trying to set the color by it's name (green or red).
This issue only affects paper.
| @@ -11,7 +11,7 @@ interface NativeProps extends ViewProps { | |||
| foreground?: boolean; | |||
| borderless?: boolean; | |||
| enabled?: WithDefault<boolean, true>; | |||
| rippleColor?: ColorValue; | |||
There was a problem hiding this comment.
Is it possible to avoid overwriting this type?
Color may also be described as a string (red or green), replacing this type with Int32 removes that possibility.
|
@latekvo Sorry for the delayed answer, but I am a bit lost to find the best fix...
Do you think it is a good idea to remove the processColor in JS ? if yes, do you know which call should be done on paper version to transform the string color to integer color on native side ? Notice that processColor return a ProcessedColorValue, not a ColorValue, I also tried to replace the type to ProcessedColorValue, not better with fabric ... |
## Description Android ripple currently does not work on `Fabric`, as all values passed to `RawButton` and it's inheritors are passed through `processColor`, which is crucial on `Paper`, and broken on `Fabric`. This PR disables usage of `processColor`, when running on `Fabric`. Note: `isFabric` cannot be moved out of the body of the `Pressable`, as it likely won't be initialised before the `Pressable` starts being rendered. More details [here](https://github.com/software-mansion/react-native-gesture-handler/blob/b3d8fd91dca195267bdc33aa20fd6f5cd37d7fe2/src/utils.ts#L47). Fixes #3246 Fixes #3312 Supersedes #3250 Likely could add a workaround for software-mansion/react-native-reanimated#6935 ## Test plan <details> <summary> Collapsed test code </summary> ```tsx import React from 'react'; import { Pressable as RNPressable, StyleSheet, Text } from 'react-native'; import { BaseButton, GestureHandlerRootView, RectButton, Pressable, } from 'react-native-gesture-handler'; const App = () => { return ( <GestureHandlerRootView> <RectButton style={styles.wrapperCustom} rippleColor={'blue'}> <Text style={styles.text}>RectButton</Text> </RectButton> <BaseButton style={styles.wrapperCustom} rippleColor={'blue'}> <Text style={styles.text}>BaseButton</Text> </BaseButton> <Pressable style={styles.wrapperCustom} android_ripple={{ color: 'blue' }}> {({ pressed }) => ( <Text style={styles.text}> {pressed ? 'Pressed!' : 'Pressable from react-native'} </Text> )} </Pressable> <RNPressable style={styles.wrapperCustom} android_ripple={{ color: 'blue' }}> {({ pressed }) => ( <Text style={styles.text}> {pressed ? 'Pressed!' : 'Pressable from react-native'} </Text> )} </RNPressable> <Pressable style={styles.wrapperCustom} android_ripple={{ color: '#00f' }}> {({ pressed }) => ( <Text style={styles.text}> {pressed ? 'Pressed!' : 'Pressable from react-native'} </Text> )} </Pressable> <RNPressable style={styles.wrapperCustom} android_ripple={{ color: '#00f' }}> {({ pressed }) => ( <Text style={styles.text}> {pressed ? 'Pressed!' : 'Pressable from react-native'} </Text> )} </RNPressable> </GestureHandlerRootView> ); }; const styles = StyleSheet.create({ container: { flex: 1, justifyContent: 'center', gap: 16, padding: 16, }, text: { fontSize: 32, }, wrapperCustom: { borderRadius: 8, padding: 6, flex: 1, backgroundColor: 'papayawhip', borderColor: 'red', borderWidth: 2, }, logBox: { flex: 1, padding: 20, margin: 10, borderWidth: StyleSheet.hairlineWidth, borderColor: '#f0f0f0', backgroundColor: '#f9f9f9', }, }); export default App; ``` </details>
Description
Fix rippleColor declaration on pressable
fix: #3246
I think colorValue can be used without processColor but here an Int32 is needed
Test plan
I tested with the sample included in the package.
I didn't retest with old architecture but no impact expected, And I already tested it sucessfully