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

Usage of @callstack/react-theme-provider #423

Merged
merged 1 commit into from
Jul 25, 2018

Conversation

souhe
Copy link
Member

@souhe souhe commented Jun 13, 2018

Replace local theme-provider with @callstack/react-theme-provider

@souhe souhe requested review from satya164 and krizzu June 13, 2018 20:38
@satya164 satya164 force-pushed the feat/new-theme-provider branch from 3826c66 to c36b6a4 Compare June 17, 2018 19:03
@callstack-bot
Copy link

callstack-bot commented Jun 17, 2018

Hey @souhe, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

@satya164 satya164 force-pushed the feat/new-theme-provider branch from c36b6a4 to 25cff4f Compare June 17, 2018 19:07
@satya164 satya164 changed the base branch from master to material-next June 17, 2018 19:07
@satya164 satya164 force-pushed the feat/new-theme-provider branch 2 times, most recently from 4f3f703 to e81c142 Compare June 17, 2018 19:17
@satya164 satya164 force-pushed the feat/new-theme-provider branch 3 times, most recently from c56b199 to 4016f8c Compare June 17, 2018 19:37
Copy link
Member

@satya164 satya164 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug in the implementation. When running the example app, you will notice that the color of the Appbar suddenly changes from purple to teal. This doesn't happen in the material-next branch.

When searching for this teal color in Paper's codebase, I found that it's used here: https://github.com/callstack/react-native-paper/blob/material-next/example/DrawerItems.js#L60, where it's passed as a prop to a component wrapped in withTheme.

From a quick look, the problem seems to be that @callstack/react-theme-provider mutates the theme object by using lodash.merge which changes the primary color to teal for every component which is visible on next re-render. react-native-paper uses the deepmerge module which returns a new object after merging instead of mutation.

@ferrannp ferrannp added the wip label Jun 21, 2018
@ferrannp
Copy link
Collaborator

@souhe how is this going?

@souhe
Copy link
Member Author

souhe commented Jun 30, 2018

@ferran @satya164 I've fixed this bug in theme-provider. This is PR with fix and unit test for the bug - callstack/react-theme-provider#21
After merge, I'll release new version, use it here and this PR should be ready to merge!

@souhe
Copy link
Member Author

souhe commented Jul 3, 2018

@satya164 @ferrannp it's done. We're using fixed version of theme-provider now

@souhe souhe removed the wip label Jul 3, 2018
@satya164 satya164 force-pushed the feat/new-theme-provider branch 2 times, most recently from f1cdee7 to bfc159b Compare July 25, 2018 12:44
@satya164 satya164 force-pushed the feat/new-theme-provider branch from bfc159b to 61ec2be Compare July 25, 2018 12:56
@satya164 satya164 merged commit 2c27321 into material-next Jul 25, 2018
@satya164 satya164 deleted the feat/new-theme-provider branch July 25, 2018 13:02
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.

5 participants