-
Notifications
You must be signed in to change notification settings - Fork 671
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
Usage of Theme UI with TypeScript #834
Comments
@deadcoder0904 modes should be under colors? Source |
Woah looks like it. I did that & it worked. Not sure where I found the above code but I vividly remember copying it from somewhere online 😂 Anything else that needs to be using TypeScript? |
I’m not sure I don’t do TypeScript. |
When you are starting a project from scratch, the more you can keep to using types the better - use |
Idk where else I need to use types in A TS guide would’ve been helpful :) |
@deadcoder0904 Do you have any specific questions? I'd love to help with them and start drafting the guide in the process. |
@hasparus basically if you include |
@hasparus any thought regarding Not TS expert here neither: what would be the recommended types to provide when overriding a preset (assuming all presets should be valid import { merge, Theme } from 'theme-ui';
import { base as preset } from '@theme-ui/presets';
export const DefaultTheme: Theme = merge(
preset as Theme, // is this safe? recommended?
{
colors: {
background: 'white',
text: 'black',
primary: 'blue'
}
} as Partial<Theme>, // is this safe?
) as Theme; // is this needed? https://codesandbox.io/s/theme-ui-typescript-theme-85jru?file=/src/CustomTheme.ts |
The perfect case answer would be You don't need to worry about it, if you misspell any variable name or mix something up, TypeScript will tell you. We ain't there yet. (@jxnblk correct me if I'm wrong) A bit off-topic right now, but the Overmind guide you linked may be relevant one day. When we're using Theme UI in statically typed codebase, 1. The general idea of theme.This one is interesting for libraries and for apps where user can edit the theme. We can't read We can use optional chaining for fonts ( 2. A particular theme, defined in the source code.It's known at compile time, so we'd like to autocomplete keys from scales when the user writes their styles for nicer DX.
This is the type we get when we write const theme = {
fonts: {
monospace: 'Fira Code'
}
} as const
type MyTheme = typeof theme (
Notice we get no autocomplete inside the theme object in snippet above. My clumsy fingers do at least two typos per second. import { Theme } from "theme-ui";
const makeTheme = <ExactTheme extends Theme>(t: ExactTheme): ExactTheme => {
return t;
}; It allows me to say "my argument const theme = makeTheme({
space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
});
export default theme;
export type MyTheme = typeof theme; We claim more control over the type of /* Type '{ spaceX: number[]; }' is not assignable to type 'Theme'.
Object literal may only specify known properties, but 'spaceX' does not exist in type 'Theme'.
Did you mean to write 'space'? ts(2322) */
const theme1: Theme = {
spaceX: [0, 4, 8, 16, 32, 64, 128, 256, 512],
}
/* totally okay, no error */
const theme2 = makeTheme({
spaceX: [0, 4, 8, 16, 32, 64, 128, 256, 512],
}) |
@flo-sch It's great you've found this. This is a bug! Even two! The first problem is -- merge is typed as what it does and not what it's supposed to be used like. Guess who's on git blame 🙋. That's correct TypeScript, but it's lousy Public API. Sorry! The signature is too universal I think. export const merge = <T>(a: Partial<T>, b: Partial<T>): T =>
deepmerge(a, b, { isMergeableObject, arrayMerge }) It isn't exactly documented as such on the webiste, but it would be useful to make it a domain specific deep merge function. /**
* Deeply merge themes
*/
export const merge = (a: Theme, b: Theme): Theme =>
deepmerge(a, b, { isMergeableObject, arrayMerge }) We should also test @theme-ui/core in TypeScript. I just noticed we don't do it. I removed your assertions and worked around the problem by passing generic parameter explicitly. (This is uncool. In every place we pass a generic parameter explicitly or do export const CustomTheme = merge<Theme>(
preset,
{
colors: {
background: "white",
text: "black",
primary: "blue"
},
alerts: {
...VARIANTS
},
buttons: {
...VARIANTS
},
messages: {
...VARIANTS
}
}
) Sweet. New error appears.
It says that So it should be either one of these strings or a nested object which corresponds to this textAlign {
color: blue;
} perfectly legal and entirely useless CSS :D @flo-sch Do you wanna do a PR for some of this or should I do it? |
Great explanations @hasparus, I never pay attention to those signature types... Need to become better at that 😅
Don't be, the project is fantastic already, thanks for all your effort in there! Your screenshot makes me wonder though: is Anyway, if you don't mind, I would be interest by trying to help with that, it feels like a good TypeScript exercice for me :D Just not sure what you mean by:
And:
|
Also, since this is an issue about TypeScript, something I think I would be interested by is to ensure the // This would be valid
<Button variant="primary">Click here</Button>
// This would not
<Button variant="someUndefinedVariantName">Click here</Button> But since type definition is by nature static and the theme dynamic (provided from the context), I guess there is no smart way to achieve that? |
That's Brent and the other guys. I just added errors :D
This could be part of this "strict mode" I think. We could typecheck this to some point if we knew what variants do you have. The hard part is the themeKey and path access: <Box variant="forms.input">
<SearchIcon />
<input {...inputProps} />
</Box> We can't do string operations on the typelevel so this is impossible without codegen. I didn't read #823 thoroughly.
Sure thing! I'd love that.
So for the second sentence. I got rid of For the first one: We didn't cover TBH, I think it's not internal anymore :D It leaked. My reasoning is:
Let's just fix the type, add a nice jsdoc comment, and describe it in the docs.
This could be the "go-to theme merging function". Need to merge some themes? That's the one. There is also a possibility that merging "design graphs" in a way that makes sense is actually harder than recursively merging objects and this also opens the way to think about it. @jxnblk what do you think about making |
If it makes usage easier, I think it could safely be exposed as the "go-to theme merging function" like you mention -- it's only a small/configured wrapper around |
Great, I will give it a try then :) |
Hey, @hasparus I just installed import {Theme} from 'theme-ui'
export const theme: Theme = {
fonts: {
body:
'system-ui, -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, "Helvetica Neue", sans-serif',
heading: '"Avenir Next", sans-serif',
monospace: 'Menlo, monospace',
},
colors: {
text: '#000',
background: 'white',
primary: '#bf1',
},
modes: {
dark: {
text: '#fff',
background: '#000',
primary: '#0cf',
},
},
} |
Another error I have is on On hover on
|
Hey @deadcoder0904, Theme is in How does the theme causing the error look like? |
@hasparus Then I'd have to install The theme looks like: import { Theme } from '@theme-ui/css'
export const theme: Theme = {
breakpoints: ['640px', '768px', '1024px', '1280px'],
space: [0, 4, 8, 16, 32, 64, 128, 256, 512],
fonts: {
body:
"system-ui, -apple-system, BlinkMacSystemFont, 'Segoe UI', Roboto, 'Helvetica Neue', Arial, 'Noto Sans', sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Noto Color Emoji'",
heading: "'Inter var', Georgia, Cambria, 'Times New Roman', Times, serif",
monospace: "Menlo, Monaco, Consolas, 'Liberation Mono', 'Courier New', monospace",
},
fontSizes: [12, 14, 16, 20, 24, 32, 48, 64],
fontWeights: {
body: 400,
heading: 700,
bold: 700,
},
lineHeights: {
body: 1.5,
heading: 1.125,
},
letterSpacings: {
body: 'normal',
caps: '0.2em',
},
colors: {
text: '#000000',
background: '#ffffff',
primary: '#87EAF2',
secondary: '#F0F4F8',
accent: '#F29B9B',
highlight: '#F7D070',
muted: '#BED0F7',
},
text: {
heading: {
fontFamily: 'heading',
lineHeight: 'heading',
fontWeight: 'heading',
},
},
styles: {
root: {
fontFamily: 'body',
lineHeight: 'body',
fontWeight: 'body',
},
h1: {
variant: 'text.heading',
fontSize: 5,
},
h2: {
variant: 'text.heading',
fontSize: 4,
},
h3: {
variant: 'text.heading',
fontSize: 3,
},
h4: {
variant: 'text.heading',
fontSize: 2,
},
h5: {
variant: 'text.heading',
fontSize: 1,
},
h6: {
variant: 'text.heading',
fontSize: 0,
},
pre: {
fontFamily: 'monospace',
overflowX: 'auto',
code: {
color: 'inherit',
},
},
code: {
fontFamily: 'monospace',
fontSize: 'inherit',
},
table: {
width: '100%',
borderCollapse: 'separate',
borderSpacing: 0,
},
th: {
textAlign: 'left',
borderBottomStyle: 'solid',
},
td: {
textAlign: 'left',
borderBottomStyle: 'solid',
},
},
} |
You should. ESLint will yell at you but the import will work. I'd install it though.
Huh, this is weird then. Both |
I think it's weird that I have to install If it helps, I just installed all packages from |
The red squiggly line on
I didn't do anything to make it go away. Just closed my laptop & slept & re-opened today to check the error was gone. Also, I did do The components/mdx/index.tsimport { H1 } from './H1'
export const mdComponents = {
h1: H1,
} components/mdx/H1.tsximport { HTMLAttributes } from 'react'
export const H1 = (props: HTMLAttributes<HTMLHeadingElement>) => (
<h1 style={{ color: 'tomato' }} {...props} />
) So I'm not sure what's the problem here? |
Oh, this one may actually be right. Did you import your ThemeProvider from @theme-ui/core? There are two ThemeProviders.
The second one is reexported from theme-ui. |
Oh I imported (1) from Goddamn, was confused for a long time. I don't think this was in the docs. Maybe a revamp is in the works? |
Closing as a discussion/several issues mentioned here have been solved |
TS noob here. I have installed
@types/theme-ui
as well as have the following intheme.ts
:How do I convert it to TS so I can get autocomplete?
I tried the following:
But I get red squiggly lines on
modes
with the errorI tried finding what should be the type in https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/theme-ui/index.d.ts but I can’t find it. Then I saw #668 to wonder if it’s even complete. I just want the
theme
object to have correct keys :)The text was updated successfully, but these errors were encountered: