BaseTheme types don't match default types from Theme-ui/css

marigold-ui / marigold

Design System based on react-aria and Tailwind CSS

https://marigold-ui.io

MIT License

101 stars 7 forks source link

BaseTheme types don't match default types from Theme-ui/css #996

Closed viktoria-schwarz closed 2 years ago

viktoria-schwarz commented 3 years ago

Description

Our self-defined BaseTheme typings don't match the theme-ui/css interface of a Theme. This can lead to unwanted behavior.

We need to look through all the types and see if they match or if we need to adjust them.

Context

make our theme accessible to use with other pre-defined themes e.g. from theme-ui

Consequences

We need to change some of the types in the theme type and accordingly in the existing themes

sebald commented 2 years ago

What's the behavior? I don't think we need to be compatible with the pre-defiend theme-ui themes. Especially since our components may have other names.

viktoria-schwarz commented 2 years ago

I thought we have talked about this once, that we need to adjust the basetheme types. But I don't mind if you say it's not needed :)

sebald commented 2 years ago

Yes, sorry that was not what I meant.

I think we should think how we structure the theme first. There shouldn't be much of an incompatibility with the theme type from theme-ui.

sebald commented 2 years ago

What I meant is, our theme should not extend the theme-ui theme.

viktoria-schwarz commented 2 years ago

Okay but should we finally change anything in our theme then?

sebald commented 2 years ago

I think so? Didn't we want to make it more strict?

viktoria-schwarz commented 2 years ago

Where do we go on from here? Our BaseTheme type allows any CSSiObjects to extend it. Where do we need to restrict the types?

sebald commented 2 years ago

It's not necessarily about restricting than hardening the type.

We could start with all the variant keys specified for the components.

E.g. "root" can have a "body" and "html" property that must be a CssObject. Same for "button", "select" and so on 😊

ti10le commented 2 years ago

I also think that it would be good to get more structure in the theme. Our theme is already very similar to the theme-ui but we have too much "freedom" when creating the individual component variants. We currently have three themes, but I have noticed that it is difficult to adapt the b2b theme for other themes. I would generalize the structure.

I would keep the basic structure at the beginning of the theme. Then for each component that has a variant, there is an object starting with the component name. Sometimes we have inside the component meaningful variants like for example the button or the alert.

But there are also some components that have only one subvariant like default or __default. Here we should use "the same" default for all or for a subvariant directly the styles in componentName: { margin: ... }

How do you imagine this @sebald @viktoria-schwarz?

sebald commented 2 years ago

It's definitely something we should look into in the next cycle. I don't know if we should off load into this ticket but rather set expectations for what the improvements should achieve

ti10le commented 2 years ago

My targets are:

should be easier to create a new theme, not just simple copying and change a few things (had it with core-theme)
Wanna know which variants are used in every theme to show only these examples in marigold docs.

ti10le commented 2 years ago

What are your targets? I think we need a few points or a list to collect all todos. Or do we need a meeting to talk about our predictions to the new structure?

ti10le commented 2 years ago

ToDo:

new Theme type in our theme.ts like theme-ui type but we want to use { } and not array [ ].
change themes that they match the new type
change variant names that every component has expressive variants
every theme should also have a clear variant structure

sebald commented 2 years ago

closed via #1563