theme-ui
theme-ui copied to clipboard
(css): Allow injecting UserTheme and typecheck colors against scale
This is IMHO the coolest PR I've sent to this repo. An actual feature!
A move towards "TypeScript: Type check property values against theme scales" v1 goal. https://github.com/system-ui/theme-ui/issues/832
I belive we can treat Theme UI as an embedded domain-specific language for branded styling.
Reasoning
Imagine you're working with strict designers. They specify what's on-brand and what's not, the list of colors, spacings etc.
We keep these design tokens in our theme scales, but we can still use arbitrary values, merge them to master, deploy and get a dissapointed slack message from a designer. What if we could constain ourselves to predefined values on type level to make sure we don't do this?
Now we can.
I kinda like the idea of the runtime working as it is now and supporting arbitrary values all the time. They are useful for prototyping, even if we don't want to merge them to master.
If we decide to not build the app on type errors, we effectively make illegal states unrepresentable.
WIP
Any help is welcome! 🙌
- [ ] Benchmark TypeScript compilation times in projects using this branch in comparison with current develop.
- [x] I'd like to refactor css/src/types a bit because it turned 800 lines of circular references. Maybe we can colocate types with the runtime code?
- [ ] We need to import more types from Emotion, get rid of the ones that were copied from DefinitelyTyped.
- Note:
Theme
depends on variants and scales. Variants depend on ThemeUIStyleObject. ThemeUIStyleObject depends onTheme
and "platform" CSS types.
- [ ] Describe it in the docs.
- [x] Tests!
@nandorojo, will this work with dripsy? I'd guess so, but I only skimmed through your codebase.
The solution is similar to
DefaultTheme injection in styled-components
:
https://styled-components.com/docs/api#create-a-declarations-file
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/styled-components/index.d.ts#L412
And declaring config in Overmind: https://overmindjs.org/core/typescript#1-declare-module.
Closes https://github.com/system-ui/theme-ui/issues/1297.
Ohh love this. If I understand correctly, I think it would close this dripsy issue? I've been thinking about the right way to do typescript intellisense a lot. This PR seems really elegant, I would definitely implement it.
Oh! I was looking for that gif!
You previously had it in the readme, am I right?
I think merging this (okay, finishing and merging, there's still some work ahead) closes the issue in Dripsy and the only thing left to do there would be an update of Theme UI version.
https://github.com/nandorojo/dripsy/blob/master/src/css/types.ts#L1-6
We don't need any codegen, the user will just opt-in to strict mode (forced design tokens mode?) with declaration merging.
Yup I put it in the readme when I was first dreaming up dripsy and didn't want to forget to work on it. I can't wait to add it back on as a feature, not an idea. Super excited. Much better to avoid code gen.
Okay, I've noticed a limitation.
Without codegen we can't concatenate strings on type level. What does it mean?
We can't statically know that
{
colors: {
gray: ["#eee", "#bbb", "#333", "#111"]
}
}
results in
gray.0
gray.1
gray.2
gray.3
so nested scales would need codegen.
We could probably do a hack like
type Color = keyof UserTheme['color'] | (string & {})
to allow arbitrary values.
Tbh I'd personally be fine with flat scales and writing gray-1
.
Does the new TypeScript 4.x tuple approach solve that?
https://devblogs.microsoft.com/typescript/announcing-typescript-4-0-beta/
(I don't know the answer.)
I typically use flat scales anyway, so I'd also be fine with that. I would prefer type consistency than nesting values.
@hasparus Hey, just checking to see if you're still planning on working on this. Really excited for it.
Relevant from Material UI: https://material-ui.com/components/about-the-lab/#typescript
I'm wondering if we should migrate to csstype
v3.
https://github.com/frenic/csstype/blob/master/index.d.ts#L18131
It uses ... | (string | {})
in every property that has ... | string
in v2, so we could just use the types from there and add the keys from exact theme to them.
@hasparus Hey, just checking to see if you're still planning on working on this. Really excited for it.
@nandorojo
Well, I am, but slowly :( Would you like to take over?
@hasparus No worries, I understand. I'm not sure I'm enough of a TS wizard for this one, but I'm happy to try to help...could you describe the steps for adding other scales?
@cmaycumber any shot you've done this kind of declaration merging before? I'm going to work off this branch to try to solve https://github.com/nandorojo/dripsy/issues/6, but I haven't done it before. (Not sure if this feature would be useful to your apps or not, but I think it's one of the more exciting things theme-ui/dripsy could add.)
@hasparus would this PR get merged if we made progress on it?
@nandorojo No, I have not. But this feature is super exciting and I'd love to see it in dripsy and theme-ui, I think it would be a great addition and would be definitely relevant in my apps/websites.
On that note, I'd definitely help as much as possible. I'll try to create a fork of this branch and work with it a bit to see if I can help.
Awesome, that's great to hear. I'm going to look into @hasparus work on this so far and try to do the same!
@hasparus is there a repository you use somewhere that has the JSDoc comments for the CSS properties? For example, where did you get this from:
export interface ColorScaleCSSProperties {
/**
* The **`color`** CSS property sets the foreground color value of an element's text and text decorations, and sets the `currentcolor` value. `currentcolor` may be used as an indirect value on _other_ properties and is the default for other color properties, such as `border-color`.
*
* **Syntax**: `<color>`
*
* **Initial value**: Varies from one browser to another
*
* | Chrome | Firefox | Safari | Edge | IE |
* | :----: | :-----: | :----: | :----: | :---: |
* | **1** | **1** | **1** | **12** | **3** |
*
* @see https://developer.mozilla.org/docs/Web/CSS/color
*/
color?: Color
...
}
I don't see it in this format on mozilla's site. Thanks!
OMG you guys are picking this up! I'm so grateful. Sorry for slow responses :c
BTW good news: TypeScript 4.1 makes strictly typing nested scales possible.
@hasparus is there a repository you use somewhere that has the JSDoc comments for the CSS properties? For example, where did you get this from...
All of these should already in Theme UI codebase. I think it's originally from CSSType, but there's no way to "inherit" these comments, so we need to copy them if we want them to appear in user tooltips.
@hasparus no worries, thanks for the response. It might make sense for one of us to move this into our own PR if you don’t plan on working on it for now.
Would you mind telling me if this is going in the right direction? https://github.com/hasparus/theme-ui/pull/1
Thanks so much for this! Really excited for this feature.
@nandorojo the Stitches PR I mentioned in Twitter DMs.
https://github.com/modulz/stitches/pull/206
Our code and tests will end up looking pretty similar I suppose.
Ah, the GIF in that PR is so satisfying. Thanks for passing this along.
good stuff everyone. so excited for this!
I should be able to finish the scales this weekend.
@hasparus The scales are all complete. Mind looking over them?
TSDoc comments aren't added yet. Figured it'd be easier to look this over without them. If anyone would be interested in adding comments so this can get merged, I'd appreciate it.
@hasparus The scales are all complete. Mind looking over them?
Getting right to it today / tomorrow morning.
I'm annotating the scales with Record<keyof ScaleProperties, 'scaleName'>
to ensure we don't miss anything.
const sizes: Record<keyof SizesCSSProperties, 'sizes'> = {}
Are we good to check those off in your original post?
yup, I think so.
I’m pretty excited about how it's starting to look.
I'd also like to get deep paths in scales to make the "strict mode" compatible with all themes.
I'm a bit worried that we need more tests and better infra (like preview releases on CI) to start adding features like this smoothly.
I'd also like to get deep paths in scales to make the "strict mode" compatible with all themes.
Agreed. I think something like this would be good:
Here's a video of it.
Code sample here.
(credit)
This is all looking good.
The only thing I'm left wondering is how to extend the sx
prop. While dripsy
follows the theme-ui spec, there are a few styles that CSS doesn't support that React Native (or React Native for Web) does.
For example, React Native for Web supports animationKeyframes
in the style prop.
Similarly, React Native's transform
looks like this:
<View
sx={{ transform: [{ translateY: 10 }] }}
/>
I added transform
to dripsy (https://github.com/nandorojo/dripsy/issues/39) to fix that, but it isn't typed. Would this raise TS errors with the sx
prop proposed in this PR? If so, is there an easy way to extend the sx
prop?
As it stands, it looks like any JSX element in a .tsx
file thinks it can accept the sx
prop. This is nice, but I'd like to be able to extend/edit that sx
interface if possible.
I'm a bit worried that we need more tests and better infra (like preview releases on CI) to start adding features like this smoothly.
I could try to test this in my app if we could get it published to a @strict
tag or something like that.
There is a similar situation with box-shadows in Dripsy, since RN doesn't use a string to define a shadow the way CSS does (see https://github.com/nandorojo/dripsy/issues/17#issuecomment-697538485).
If so, is there an easy way to extend the sx prop?
I think we can use declaration merging for that. I'll try that and send you a snippet.
Awesome, thanks.
@hasparus is there anything we're missing here to move towards a merge?
@millsp has been incredibly helpful here with giving insight on generating template literal strings from an object type. It seems like we could use his solution for adding strict nested fields with TS 4.1.
@nandorojo @lachlanjc would you have a moment to give this PR another look? @beerose you promised me code review.
There should be no breaking changes at runtime, and I think there's also no breaking changes on type level.