-
-
Notifications
You must be signed in to change notification settings - Fork 81
feat(iOS): add customIcon and customIconName properties to the action object #111
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thank you for this! I will review soon and be in touch! |
okay. let me know if there is a better way converting the color. as i understood a direct prop of a view can be converted using this macro: |
Before merging I want to make sure there is not a better way to pass the color to iOS like we do on android. I also am thinking about the API, since Android supports this exact same workflow so they should probably use the same props to do so. I think Thoughts? |
yeah, now the passing of the color is a bit messy for iOS with the whole processColor, but as a last resort we could just use the color converter, that i mentioned, which is just a few lines of code and works really well for hex colors. but then it doesn't support react-native style colors like sounds good to me, i thought about it too, but didn't want to mess with the previously set prop names too much, as it will break other people's code with the refactoring. but i can change the names for both iOS and Android to just |
okay i refactored the prop names and made some modifications to the README. in the type declarations i marked iconColor with string only, if we can process the color somehow in iOS on the native side it should stay like that. the example app is unchanged now, when we will have a mergeable PR we can change that to fit the up to date API |
This is clean work, nice job |
This is awesome work. For
const ContextMenu = (props) => {
const iconColor = props.iconColor ? processColor(props.iconColor) : undefined;
return (
<NativeContextMenu {...props} iconColor={iconColor}>
{props.children}
{props.preview != null && Platform.OS === 'ios' ? (
<View style={styles.preview} nativeID="ContextMenuPreview">{props.preview}</View>
) : null}
</NativeContextMenu>
);
}; |
okay, this should be good enough. i added this change, but checked for iOS, because on Android i think you can pass a react-native color normally. right now i won't have much time, but later at night i will test it and change the example app to fit the new changes :) |
All in all, this is great. Thanks for the awesome contribution |
Hello there. I just added two new properties to the actions object:
customIcon
andcustomIconColor
. There are some things that are good to know:systemIcon
andcustomIcon
is provided,customIcon
will overwrite the icon specified insystemIcon
customIconColor
only applies tocustomIcon
, i tried applying it tosystemIcon
too, but with no successprocessColor()
fromreact-native
, when you add thecustomIconColor
prop (ex.actions={[{ customIconColor: processColor('#385858') }]})
. as i am not too familiar withobjective-c
, i don't know how can you provide the string specified on theJS side
as an integer toRCTConvert
, which should give us anUIColor
after converting said integer. in my personal project i don't useprocessColor()
, as i implemented the hex conversion on the native side with:but maybe the simplistic native side code would be too bloated with this.
Anyways, please tell me if something needs to be modified or if there is a better way to implement this, i take advices happily.