theme-ui icon indicating copy to clipboard operation
theme-ui copied to clipboard

(css): Allow injecting UserTheme and typecheck colors against scale

Open hasparus opened this issue 3 years ago • 53 comments

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 on Theme 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.


image


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.

hasparus avatar Jul 26 '20 19:07 hasparus

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.

nandorojo avatar Jul 26 '20 19:07 nandorojo

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.

hasparus avatar Jul 26 '20 20:07 hasparus

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.

nandorojo avatar Jul 26 '20 20:07 nandorojo

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.

hasparus avatar Jul 29 '20 15:07 hasparus

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.

nandorojo avatar Aug 03 '20 00:08 nandorojo

@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

nandorojo avatar Aug 10 '20 19:08 nandorojo

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 avatar Aug 11 '20 22:08 hasparus

@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?

nandorojo avatar Aug 23 '20 14:08 nandorojo

@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 avatar Sep 30 '20 17:09 nandorojo

@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.

cmaycumber avatar Sep 30 '20 17:09 cmaycumber

Awesome, that's great to hear. I'm going to look into @hasparus work on this so far and try to do the same!

nandorojo avatar Sep 30 '20 17:09 nandorojo

@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!

nandorojo avatar Sep 30 '20 17:09 nandorojo

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 avatar Sep 30 '20 18:09 hasparus

@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 avatar Sep 30 '20 18:09 nandorojo

@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.

hasparus avatar Sep 30 '20 19:09 hasparus

Ah, the GIF in that PR is so satisfying. Thanks for passing this along.

nandorojo avatar Sep 30 '20 19:09 nandorojo

good stuff everyone. so excited for this!

samburgers avatar Oct 10 '20 21:10 samburgers

I should be able to finish the scales this weekend.

nandorojo avatar Oct 11 '20 02:10 nandorojo

@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.

nandorojo avatar Oct 12 '20 18:10 nandorojo

@hasparus The scales are all complete. Mind looking over them?

Getting right to it today / tomorrow morning.

hasparus avatar Oct 14 '20 14:10 hasparus

I'm annotating the scales with Record<keyof ScaleProperties, 'scaleName'> to ensure we don't miss anything.

const sizes: Record<keyof SizesCSSProperties, 'sizes'> = {}

hasparus avatar Oct 16 '20 10:10 hasparus

Are we good to check those off in your original post?

nandorojo avatar Oct 16 '20 18:10 nandorojo

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.

hasparus avatar Oct 16 '20 20:10 hasparus

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:

Screen Shot 2020-10-16 at 4 47 42 PM

Here's a video of it.

Code sample here.

(credit)

nandorojo avatar Oct 16 '20 20:10 nandorojo

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

image

nandorojo avatar Oct 16 '20 21:10 nandorojo

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.

hasparus avatar Oct 16 '20 21:10 hasparus

Awesome, thanks.

nandorojo avatar Oct 19 '20 13:10 nandorojo

@hasparus is there anything we're missing here to move towards a merge?

nandorojo avatar Oct 31 '20 20:10 nandorojo

@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 avatar Nov 21 '20 18:11 nandorojo

@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.

hasparus avatar Dec 02 '20 19:12 hasparus