-
Notifications
You must be signed in to change notification settings - Fork 673
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
Convert @theme-ui/theme-provider to TypeScript #708
Conversation
LGTM generally, but there's a small issue I need to investigate (not sure how to unnaprove without requesting changes). GitHub Actions didn't run on this PR. I also have two minor issues but I don't think we should address them here.
|
packages/theme-provider/src/index.ts
Outdated
import { ColorModeProvider } from '@theme-ui/color-modes' | ||
import { MDXProvider } from '@theme-ui/mdx' | ||
import { Global } from '@emotion/core' | ||
|
||
const BodyStyles = () => | ||
jsx(Global, { | ||
styles: theme => { | ||
styles: (theme: Theme) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails in tests, because Global.styles argument is unknown
and it's not assignable to theme
.
We'd either need to do
<Global<Theme> styles={correctlyTypedTheme => ({})}>
or
jsx(Global, {
styles: emotionTheme => {
const theme = emotionTheme as Theme
//...
},
});
Both are in fact assertions. The second one is just explicit. I'll leave it here as a suggestion.
styles: (theme: Theme) => { | |
styles: emotionTheme => { | |
const theme = emotionTheme as Theme |
806ebb0
to
b5a664d
Compare
Hey @epilande, I took the liberty of fixing merge conflicts and few minor test problems. @jxnblk @mxstbr |
Sidenote: theme-ui/packages/theme-provider/src/index.ts Lines 34 to 58 in 076cd7b
I'm not sure if It should actually be okay now if @epilande Do you think it makes sense? |
Convert
@theme-ui/theme-provider
to TypeScript.This PR needs #707.
Related to #668.