Skip to content

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

Merged
hasparus merged 7 commits into
system-ui:masterfrom
epilande:ts-theme-provider
May 11, 2020
Merged

Convert @theme-ui/theme-provider to TypeScript#708
hasparus merged 7 commits into
system-ui:masterfrom
epilande:ts-theme-provider

Conversation

@epilande

@epilande epilande commented Feb 23, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
epilande
epilande commented Feb 23, 2020
View reviewed changes
Comment thread packages/css/src/index.ts Outdated
@mxstbr mxstbr mentioned this pull request Mar 1, 2020
Closed
31 of 32 tasks
hasparus
hasparus approved these changes May 8, 2020 •
edited
Loading
View reviewed changes
@hasparus

hasparus commented May 8, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

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.

hasparus
hasparus requested changes May 8, 2020
View reviewed changes
Comment thread packages/theme-provider/test/index.tsx
hasparus
hasparus reviewed May 8, 2020
View reviewed changes
Comment thread packages/theme-provider/src/index.ts Outdated
const BodyStyles = () =>
jsx(Global, {
styles: theme => {
styles: (theme: Theme) => {

@hasparus hasparus May 8, 2020

Copy link
Copy Markdown
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
hasparus force-pushed the ts-theme-provider branch from 806ebb0 to b5a664d Compare May 10, 2020 11:06
@hasparus
hasparus self-requested a review May 10, 2020 11:07
@hasparus

hasparus commented May 10, 2020

Copy link
Copy Markdown
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

hasparus commented May 12, 2020

Copy link
Copy Markdown
Member

Sidenote:

theme-ui/packages/theme-provider/src/index.ts

Lines 34 to 58 in 076cd7b

})
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

Reviewers

@jxnblk jxnblk jxnblk left review comments
@mxstbr mxstbr mxstbr approved these changes
@hasparus hasparus hasparus approved these changes
+1 more reviewer
@8eecf0d2 8eecf0d2 8eecf0d2 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@epilande @hasparus @jxnblk @8eecf0d2 @mxstbr