alessiocancian / react-native-actionsheet

An elegant ActionSheet component for React Native.
MIT License
63 stars 24 forks source link

tint color prop not working correctly on android #16

Closed 0had0 closed 2 years ago

0had0 commented 2 years ago

Hi! πŸ‘‹

Firstly, thanks for your work on this project! πŸ™‚

Today I used patch-package to patch @alessiocancian/react-native-actionsheet@3.1.1 for the project I'm working on.

Here is the diff that solved my problem:

diff --git a/node_modules/@alessiocancian/react-native-actionsheet/lib/ActionSheetCustom.js b/node_modules/@alessiocancian/react-native-actionsheet/lib/ActionSheetCustom.js
index 548e08b..fea76e0 100644
--- a/node_modules/@alessiocancian/react-native-actionsheet/lib/ActionSheetCustom.js
+++ b/node_modules/@alessiocancian/react-native-actionsheet/lib/ActionSheetCustom.js
@@ -140,7 +140,7 @@ class ActionSheet extends React.Component {
   _createButton (title, index) {
     const styles = getMergedStyles(this.props.styles)
     const { buttonUnderlayColor, cancelButtonIndex, destructiveButtonIndex, disabledIndexes, disabledColor } = this.props
-    const tintColor = this.props.tintColor || this._isDarkMode() ? "#4793FF" : "#007AFF"
+    const tintColor = this.props.tintColor ?? (this._isDarkMode() ? "#4793FF" : "#007AFF")
     const fontColor = destructiveButtonIndex === index ? WARN_COLOR : tintColor
     const isCancel = cancelButtonIndex === index
     const buttonBoxStyle = isCancel ? styles.cancelButtonBox : styles.buttonBox

This issue body was partially generated by patch-package.

tannermares commented 2 years ago

@alessiocancian I'm seeing this issue as well. Anything blocking this fix from being added?

tannermares commented 2 years ago

@alessiocancian would it help you if a PR was made with this change? If you approve of this fix I'd be happy to make one.

alessiocancian commented 2 years ago

Sorry I'd like to approve the PR, my only concern is that nullish coalescing operator is not supported "out of the box" in old Typescript and Babel versions, so it might cause problems for someone working on legacy projects (even if easy to fix by updating libraries or adding the babel plugin). So I'd like to keep || instead of ??, it should work the same in this case but I haven't tested it yet, if you confirm it still works I'll publish the fix this evening or tomorrow.

tannermares commented 2 years ago

@alessiocancian something like this should work without relying on ?? right?

this.props.tintColor ? this.props.tintColor : this._isDarkMode() ? "#4793FF" : "#007AFF"
alessiocancian commented 2 years ago

@alessiocancian something like this should work without relying on ?? right?

this.props.tintColor ? this.props.tintColor : this._isDarkMode() ? "#4793FF" : "#007AFF"

That's the same of using the || operator, the result with ?? is slightly different because empty strings or 0 numbers are not considered falsy, but it shouldn't be a problem with this prop. This is a precedence issue because the OR operator is executed before the ternary operator, so adding the parentheses after the || should be enough to fix.

tannermares commented 2 years ago

@alessiocancian ah yes, I didn't realize you meant keeping the parens. That sounds right to me!

alessiocancian commented 2 years ago

@tannermares I released it to npm, please test v3.1.2 and tell me if it works

tannermares commented 2 years ago

@alessiocancian all good on my end! Thanks for the fix!