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

Convert @theme-ui/theme-provider to TypeScript #708

Merged
merged 7 commits into from
May 11, 2020

Conversation

epilande
Copy link
Contributor

@epilande epilande commented Feb 23, 2020

Convert @theme-ui/theme-provider to TypeScript.

This PR needs #707.

Related to #668.

@epilande epilande requested a review from mxstbr February 23, 2020 10:39
packages/css/src/index.ts Outdated Show resolved Hide resolved
@mxstbr mxstbr mentioned this pull request Mar 1, 2020
32 tasks
hasparus
hasparus approved these changes May 8, 2020
@hasparus
Copy link
Member

hasparus commented May 8, 2020

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.

  • There's a missing dependency on theme-provider -> typography which, I think, will fail microbundle after babel packages update.
  • The colorMode and setColorMode could be merged to ContextValue in @theme-ui/color-modes instead of being defined in core ahead of time.

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) => {
Copy link
Member

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.

Suggested change
styles: (theme: Theme) => {
styles: emotionTheme => {
const theme = emotionTheme as Theme

@hasparus
Copy link
Member

Hey @epilande, I took the liberty of fixing merge conflicts and few minor test problems.

@jxnblk @mxstbr
I followed up on #708 (comment), and made JS tests and examples reflect that the theme prop is now expected.

@hasparus hasparus merged commit 076cd7b into system-ui:master May 11, 2020
@hasparus
Copy link
Member

Sidenote:

})
interface ThemeProviderProps {
theme: Theme
children?: React.ReactNode
components?: { [key in keyof IntrinsicSxElements]?: React.ReactNode }
}
export const ThemeProvider: React.FC<ThemeProviderProps> = ({
theme,
components,
children,
}) => {
const outer = useThemeUI()
if (typeof outer.setColorMode === 'function') {
return jsx(
CoreProvider,
{ theme },
jsx(MDXProvider, {
components,
children,
})
)
}

I'm not sure if keyof IntrinsicSxElements here isn't too little. i.e. it's passed to MDXProvider so I think it can actually be any string. Whatever we pass will get used in MDX.

It should actually be okay now if components are defined separately and then passed,
but if someone defines them inline in JSX, she'll get an "extra keys" error.

@epilande Do you think it makes sense?

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.

None yet

5 participants