-
Notifications
You must be signed in to change notification settings - Fork 2.9k
ThemeProvider: Export ThemeContext #14803
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
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 55b6d19:
|
Asset size changes
Baseline commit: cf72dc9b1b66b9c5f383d4607cb4adfb80c03895 (build) |
|
As per our conversation from Teams, it would be a really cool feature to programmatically define what the "default" value of ThemeContext is. Right now it's "undefined", which because we're using Typescript is always a pain to handle. We can do this manually by wrapping ThemeContext.Consumer and checking if 'theme' is undefined (aka no Provider) and then providing our own app's "default theme". It would just be a bonus to be able to tell ThemeContext to take our app's "default theme" as the default value Not even sure this is possible with React's Context, but something to look into :) |
|
@kelseyyoung - I actually need to distinguish if the context is default or it's provided using |
|
@xugao well like I said it's not a huge thing. I'm thinking our solution looks like this: And because we're abstracting this out into a singular "component", if things change we can make that change in one place. That way the signature of getStyles can be:
and we get rid of the "undefined". So this isn't hard for us. Having a way to programmatically say "if the consumer has no theme, fill in our own" would also be cool. But really just a nice to have So I agree with what you have now being fine as well |
|
🎉 Handy links: |
* export themecontext * Change files * nit
Pull request checklist
$ yarn changeDescription of changes
Export
ThemeContextand add documentation in READMEFocus areas to test
(optional)