Skip to content

[V4] fix themes related examples #571

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

Merged
merged 2 commits into from
Jan 23, 2020
Merged

Conversation

Simek
Copy link
Contributor

@Simek Simek commented Nov 8, 2019

This PR fixes small issues in themes related examples :

  • missing theme variable initialization in static context usage example
  • missing context initialization in "Themes inside navigationOptions"
  • missing ThemeContext import in "Theming tabs and other similar navigators"

@satya164 satya164 force-pushed the source branch 5 times, most recently from 8b02cfa to e7a8a2c Compare November 13, 2019 19:45
@WoLewicki
Copy link
Member

Thanks for contribution! I think we should add ThemeConstants to "Themes inside navigationOptions" to be consistent and don't import ThemeContext in "Theming tabs and other similar navigators" but rather create it as in "Themes inside navigationOptions". Could you do it? And what do you think @satya164 ?

@satya164
Copy link
Member

User needs to import ThemeContext to be able to use the theme from React Navigation. Creating own theme context means it's a different theme system. I think user shouldn't create separate context if they just need to integrate with React Navigation's themes.

@WoLewicki
Copy link
Member

Yes, but in the examples, there are used custom values from ThemeConstants, like this
let currentTheme = ThemeConstants[screenProps.theme];

@satya164
Copy link
Member

I'm not following. I mean current ThemeConstants usage is ok, but the ThemeContext should be imported instead of being created by the user.

@WoLewicki
Copy link
Member

Ok, so just change the creation of ThemeContext to importing it from react-navigation ?

@satya164
Copy link
Member

Yeah

@Simek
Copy link
Contributor Author

Simek commented Jan 12, 2020

@WoLewicki @satya164 Thank you for the comments.

I have modified the PR according to your discussion. I hope I have understood the required change properly.

@WoLewicki
Copy link
Member

Thanks for the update! I think it is okay now, if @satya164 thinks so too, we can merge it.

@WoLewicki WoLewicki merged commit bb10637 into react-navigation:source Jan 23, 2020
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.

3 participants