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

Fixed nested themes not being republished on outer theme changes #497

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Dec 20, 2017

What:

This fixes a bug when having nested ThemeProviders and updating outer theme the change is not reaching the StyledComponent

<ThemeProvider theme={{}}>
	<ThemeProvider theme={{}}>
		<StyledComponent>
	</ThemeProvider>
</ThemeProvider>

Why:

This is a bug fix.

How:
Using publish in the nested ThemeProviders subscriptions.

Checklist:

  • Documentation - N/A
  • Tests
  • Code complete

@emmatown
Copy link
Member

emmatown commented Dec 20, 2017

emotion-theming is basically the same as styled-components' theming so I think this bug might also exist in styled-components.

I also checked and it seems like this would also happen in glamorous too.

cc @kentcdodds @mxstbr

Edit: Just reproduced it with glamorous and styled-components.

@Andarist Andarist force-pushed the fix/nested-theme-providers branch 2 times, most recently from 369ad6f to baf8f43 Compare December 21, 2017 00:39
@Andarist
Copy link
Member Author

Nice catch! I've sent out PRs to both, still need to sit down to write a test and port it to all 3 repos.

Those ThemeProviders are basically the same in all packages, wouldn't it make sense to join forces and have a single ThemeProvider package to maintain?

@codecov
Copy link

codecov bot commented Dec 21, 2017

Codecov Report

Merging #497 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted Files Coverage Δ
packages/emotion-theming/src/theme-provider.js 100% <100%> (ø) ⬆️

@emmatown
Copy link
Member

emmatown commented Dec 21, 2017

@Andarist There's been an effort to do that in https://github.com/cssinjs/theming but it isn't the exact API we need, it's not as performant and no one has been particularly motivated to change it.

@Andarist Andarist force-pushed the fix/nested-theme-providers branch from baf8f43 to 8af74e3 Compare December 21, 2017 11:07
@Andarist
Copy link
Member Author

@mitchellhamilton I've added a test so the PR is ready to merge (I think 😄 )

There's been an effort to do that in https://github.com/cssinjs/theming but it isn't the exact API we need, it's not as performant and no one has been particularly motivated to change it.

That's a bummer, seems really sad having to reimplement this fix in every lib when their codebases around this component is 90+% the same.

Copy link
Member

@emmatown emmatown left a comment

Choose a reason for hiding this comment

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

Thanks!!

@emmatown emmatown merged commit d140fb4 into emotion-js:master Dec 21, 2017
@Andarist Andarist deleted the fix/nested-theme-providers branch December 21, 2017 11:42
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.

2 participants