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
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