Skip to content
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

Add props to headerLeft and headerRight #500

Merged
merged 5 commits into from May 22, 2020
Merged

Add props to headerLeft and headerRight #500

merged 5 commits into from May 22, 2020

Conversation

AzzouQ
Copy link
Contributor

@AzzouQ AzzouQ commented May 7, 2020

This PR will allow users to custom headerRight and headerLeft components using user's theme color.
For example, if I want a custom settings icon in headerRight to navigate to my settings page, I will be able to retrieve color from my custom theme. It's already available in ReactNavigation.

colors.primary is used by the back button so it makes sense to use it for headerRight and headerLeft.

The same logic may be applied to headerCenter (Imagine a custom TextComponent), we may apply colors.text in this case.

Let me know if you find this useful (or not), or if I can make it better to be integrated.

@WoLewicki
Copy link
Member

I think it would be best if you replicated the behavior of color={headerTintColor !== undefined ? headerTintColor : colors.primary}. Also could you add the same thing for the headerCenter?

@AzzouQ
Copy link
Contributor Author

AzzouQ commented May 21, 2020

@WoLewicki Done, I add it to headerCenter with colors.text too. I personally prefer to use ?? operators (as defined here https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing_operator) but if you want me to change back to the classic a !== undefined ? a : b, there's no problem.

@WoLewicki
Copy link
Member

Please change it to the classic form so we can keep it consistent. Maybe we will change all the occurrences to nullish coalescing soon. Also could you please use the colors.primary in the headerCenter too? It seems more fitting for every custom view to get the same default color.

@WoLewicki
Copy link
Member

Or we can leave it as it is, nevermind. Will change headerCenter only and merge it since we are releasing the library.

@WoLewicki WoLewicki merged commit 867b1f2 into software-mansion:master May 22, 2020
@AzzouQ
Copy link
Contributor Author

AzzouQ commented May 22, 2020

Wouldn’t it be more logical for headerCenter to mimic headerTitle behavior ? headerCenter will most likely be a custom TextComponent, so colors.text would make more sense ? Anyway, glad it’s getting merged 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants