Sb/add theme basics #725
Sb/add theme basics #725
Conversation
|
@WilliamKelley, what are your thoughts on adding the theme so far? Right direction? |
There was a problem hiding this comment.
@WilliamKelley, what are your thoughts on adding the theme so far? Right direction?
I'm not sure, what is "correct" to do to the theme depends on (1) how the configurated values affect components and (2) the structure and properties of the theme that is available to consumers.
Consumers should expect theme object that matches or is compatible with all our design system's properties. Knowing the best way to achieve that while customizing components requires that you have a component developed with our desired Props API and Styles. That's why I'm suggesting you do this only in tandem with a Button.
| brand: { | ||
| blue: string; | ||
| lightBlue: string; | ||
| }; |
There was a problem hiding this comment.
Does this not have to be brand: PaletteRange? That's what the docs would suggest. Can you link in your PR description to whatever you're following? That's the only what i can judge correct-ness.
There was a problem hiding this comment.
Also this seems like it would expose color="brand" as an option for color-enabled components. But "brand" is not a valid color for us and is not our desired API. So this would be a wrong way to get these values (blue, light blue) in the theme
There was a problem hiding this comment.
I tried PalletteRange and got the following error when I tried to set a value to blue:
Type '{ blue: string; lightBlue: string; }' is not assignable to type '{ 50?: string | undefined; 100?: string | undefined; 200?: string | undefined; 300?: string | undefined; 400?: string | undefined; 500?: string | undefined; 600?: string | undefined; 700?: string | undefined; ... 27 more ...; solidDisabledBg?: string | undefined; }
Which is why I went with being specific to each one.
| }, | ||
| }, | ||
| fontFamily: { | ||
| body: '"Poppins", sans-serif', |
There was a problem hiding this comment.
Poppins is only used for headings. Normal body text is Inter.
There was a problem hiding this comment.
Right, I was putting something there as a placeholder. Where is all of the body/h1 => font documented?
I looked here
There was a problem hiding this comment.
There's no defined mapping because there shouldn't be. See my comment in Figma on yours. Variants and the underlying html tag (component) are rightfully independent.
|
Something more basic might be Typography, if you just want to tackle that first. |
Initial code for expanding a theme.