Open krijoh92 opened 2 years ago
krijoh92
commented
2 years ago A side question I have: Have you considered migrating away from js and only develop in typescript? I might make it easier to infer types from all components, theme, etc if you're implementing everything in typescript from the get-go?
brandongregoryscott
commented
2 years ago Would it be sensible to inlcude those components in the type? The components I found missing were: DialogBody, DialogFooter, DialogHeader, Option and Table
Table was probably an oversight, and the rest were probably missed because they aren’t really “first-class” components or meant to be used on their own. (Option is used internally in the SelectMenu and Autocomplete components, and while we do export it from index.js, it probably doesn’t have a lot of use on its own). However, I don’t see the harm in adding them to the type alias since they are themed like regular components are.
Speaking of which, I’ve been thinking for a while now that DialogBody, DialogFooter and DialogHeader should be exposed as their own components tacked on to the Dialog object - similar to Menu.Item, Menu.Group or the EmptyState.PrimaryButton extension components - I’ve lost count of how many times I’ve needed to slightly tweak a Dialog header and had to reference the source for spacing/styling. I can spin up a separate issue for this. (#1466)
Would it be sensible to expand the theme type to allow those functions? Atm I’m overriding the theme type to allow it in our project.
AFAIK, these theme callback functions are remnants from a previous iteration of the theming architecture. There’s still internal theme objects that use it, but I’m not sure it’s a pattern we still want to move forward with or encourage others to use externally - do you have specific use-cases in mind where this sort of function would help you do something the standard theming conventions wouldn’t?
A side question I have: Have you considered migrating away from js and only develop in typescript? I might make it easier to infer types from all components, theme, etc if you’re implementing everything in typescript from the get-go?
We’d love to - it would certainly make the typing aspect of theming (and all things) much smoother and more accurate overall. (In the most recent theme typing improvements, I manually went through and documented all of the pseudoselectors available in the default theme - that wasn’t very fun) As you can imagine, porting over a codebase of this size is no small feat, even with tooling like ts-migrate available.
I’ve got a running branch on my fork that has the vast majority ported over, but there’s still a lot of testing + some polish before it’ll be ready to merge in and release.
krijoh92
commented
2 years ago Thanks for the detailed response Brandon! I know I would prefer that not just "first-class" components are able to be "themed". I would rather be able to have fine-grained control over all components through the theme if able.
Our specific use-cases for using the callback function are when we want to style something based on a combination of props. Example is our primary buttons with specific intents can be colored correctly etc. Not sure if there are any other good ways to do so, but at least that is the way we found to get it to work atm.
Another use-case was to basically calculate variations of colors from our "common" theme. We went for doing that over enumerating all of the specific colors in multiple themes and so we wouldn't pollute theme.colors with a lot of colors.
As you can imagine, porting over a codebase of this size is no small feat
Yeah I know exactly what you mean, I spent a couple of months doing this exact thing last year.
Side question: Do you know how one would go about to set specific styling for the spinner that appears in buttons that have isLoading ? Haven't figured a way to do it, and our regular spinner color is basically invisible when our primary button is loading.
brandongregoryscott
commented
2 years ago I know I would prefer that not just "first-class" components are able to be "themed". I would rather be able to have fine-grained control over all components through the theme if able.
Totally understandable. PRs welcome - I believe adding these to the Components type alias should 'just work' and let you define overrides for those specific object keys.
Our specific use-cases for using the callback function are when we want to style something based on a combination of props. Example is our primary buttons with specific intents can be colored correctly etc.
Ah, I see. That's fair. The typing for this might be a little more complex - from what I've gathered, the object passed as the second param in that callback there isn't really the full component props object but an object of the "modifier" props like intent or appearance, basically what we pass as the second argument to useStyleConfig:
https://github.com/segmentio/evergreen/blob/108a5bc9392db62b6a9f8f177a3a7fa0054c6689/src/hooks/use-style-config.js#L125-L133
Side question: Do you know how one would go about to set specific styling for the spinner that appears in buttons that have isLoading ?
I don't think that's possible right now with the way the Button is built - we'd need to pass through some color prop to the Spinner it renders (which it doesn't support as a prop now regardless), I think:
https://github.com/segmentio/evergreen/blob/108a5bc9392db62b6a9f8f177a3a7fa0054c6689/src/buttons/src/Button.js#L103-L109
Missing components in exported 'Components' type
When checking the value of the defaultTheme runtime, I saw a couple of components that I would like to override that weren't included in the
type Componentsyou export from the typings.Would it be sensible to inlcude those components in the type? The components I found missing were: DialogBody, DialogFooter, DialogHeader, Option and Table
Missing typing for theme callback function
I've seen that some components have functions to apply styles in the default theme and experimented with it, and what I found was that the callbackfunction basically has a signature of
(theme: Theme, props: ComponentProps)where "ComponentProps" is the given component's props. Would it be sensible to expand the theme type to allow those functions? Atm I'm overriding the theme type to allow it in our project.