[ActionSheetIOS] Add tintColor for buttons. #4590
Conversation
|
By analyzing the blame information on this pull request, we identified @nicklockwood, @vjeux and @tadeuzagallo to be potential reviewers. |
facebook-github-bot
added
GH Review: review-needed
CLA Signed
| @@ -12,6 +12,7 @@ | |||
| 'use strict'; | |||
|
|
|||
| var RCTActionSheetManager = require('NativeModules').ActionSheetManager; | |||
| var processColor = require('processColor'); | |||
There was a problem hiding this comment.
Move this under invariant, keep them grouped and alphabetically ordered
christopherdro
force-pushed
the
actionsheet-tint
branch
from
e800259 to
9b3e995
Compare
|
@christopherdro updated the pull request. |
facebook-github-bot
added
GH Review: review-needed
and removed
GH Review: needs-revision
labels
|
@ide Thanks for your comments. Just updated the PR with a few of those changes. |
ide
added
GH Review: accepted
Platform: iOS
|
|
||
| if (processedTintColor) { | ||
| options.tintColor = processedTintColor; | ||
| } |
There was a problem hiding this comment.
I think this can just be collapsed to
options.tintColor = processColor(options.tintColor);
There was a problem hiding this comment.
Yea this was made based on @ide comment here.
#4590 (comment)
There was a problem hiding this comment.
He's right that you shouldn't call processColor() multiple times, but I also don't think there's any benefit to checking its result before assigning it.
In the code as you've written it, if options.tintColor is some invalid string that causes processedTintColor to return null or zero, you're actually leaving options.tintColor as the original string instead of overwriting it with transparent, which will then crash the native code that's expecting a number instead of a string.
It's perfectly safe to just write
options.tintColor = processColor(options.tintColor);
That should ensure that whatever garbage value options.tintColor goes in as, it will come out as a sanitised value. If for some reason it doesn't, that should be fixed in processColor.js, not in ActionSheetIOS.js.
There was a problem hiding this comment.
@nicklockwood Thanks again for taking a look.
christopherdro
force-pushed
the
actionsheet-tint
branch
from
9b3e995 to
b0106dc
Compare
christopherdro
force-pushed
the
actionsheet-tint
branch
from
b0106dc to
54cda40
Compare
|
@christopherdro updated the pull request. |
|
@facebook-github-bot import |
|
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/146659502363867/int_phab to review. |
Summary: This pr adds documentation for the tintColor addition of #4590 <!-- Thank you for sending the PR! We appreciate you spending the time to work on these changes. Help us understand your motivation by explaining why you decided to make this change. You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html Happy contributing! --> The tintColor was missing from the documentation but works perfectly fine. Added a tintColor to showActionSheetWithOptions and showShareActionSheetWithOptions in the app i am building right now. [DOCS][MINOR][ActionSheetIOS] - Added documentation for tintColor in ActionSheet. Closes #16679 Differential Revision: D6248070 Pulled By: shergin fbshipit-source-id: a2276f50b42ff2c5858008f3641c9607f248744a
Summary: This pr adds documentation for the tintColor addition of facebook#4590 <!-- Thank you for sending the PR! We appreciate you spending the time to work on these changes. Help us understand your motivation by explaining why you decided to make this change. You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html Happy contributing! --> The tintColor was missing from the documentation but works perfectly fine. Added a tintColor to showActionSheetWithOptions and showShareActionSheetWithOptions in the app i am building right now. [DOCS][MINOR][ActionSheetIOS] - Added documentation for tintColor in ActionSheet. Closes facebook#16679 Differential Revision: D6248070 Pulled By: shergin fbshipit-source-id: a2276f50b42ff2c5858008f3641c9607f248744a
* Document `tintColor` option for ActionSheetIOS also: - fixed the `showActionSheetWithOptions` example This option was added in facebook/react-native#4590 FYI, this is not even documented in the comments https://github.com/facebook/react-native/blob/master/Libraries/ActionSheetIOS/ActionSheetIOS.js#L28 Are the docs generated from the code? Should I update that instead? * add tintColor option in docs/ folder as well
* Document `tintColor` option for ActionSheetIOS also: - fixed the `showActionSheetWithOptions` example This option was added in facebook/react-native#4590 FYI, this is not even documented in the comments https://github.com/facebook/react-native/blob/master/Libraries/ActionSheetIOS/ActionSheetIOS.js#L28 Are the docs generated from the code? Should I update that instead? * add tintColor option in docs/ folder as well
* Document `tintColor` option for ActionSheetIOS also: - fixed the `showActionSheetWithOptions` example This option was added in facebook/react-native#4590 FYI, this is not even documented in the comments https://github.com/facebook/react-native/blob/master/Libraries/ActionSheetIOS/ActionSheetIOS.js#L28 Are the docs generated from the code? Should I update that instead? * add tintColor option in docs/ folder as well
Closes #3374