-
Notifications
You must be signed in to change notification settings - Fork 32
fix: update type definitions to use ConditionalValue for color palette props #362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Also, as official documentation states https://www.chakra-ui.com/guides/theming-change-default-color-palette, the default colorPalette is |
Hey thanks for the PR! First, I just want to point out that you can use custom However, I am thinking you're right that the export interface SystemProperties {
// ...
colorPalette?: ConditionalValue<UtilityValues["colorPalette"] | CssVars | AnyString>
// ...
} And if I follow the types up the chain, I see the following: type AnyString = string & {}
type CssVars = `var(--${string})`
export interface UtilityValues {
colorPalette:
| "transparent"
| "current"
| "black"
| "white"
| "whiteAlpha"
| "blackAlpha"
| "gray"
| "red"
| "orange"
| "yellow"
| "green"
| "teal"
| "blue"
| "cyan"
| "purple"
| "pink"
| "brand"
| "bg"
| "fg"
| "border"
// ...
} From what I can tell, the Now the one big problem with your ![]() And this is what it looks like with your version: ![]() If you're curious, there's a large thread about the on the typescript repo: microsoft/TypeScript#29729 Besides that, I'd generally prefer the type of the prop for this package to be as close as possible to the version from Chakra themselves. So I think something like the following would work. First, add the following to the export type CssVars = `var(--${string})`;
export type AnyString = string & {};
export type ColorPaletteProp = ConditionalValue<
ColorPalette | CssVars | AnyString
>; Unfortunately, the Then change the types of all of the If you make these changes, I'd be happy to merge! EDIT: There are actually a couple more instances where |
As for this, I'd consider it as well. I think the main thing is that I'd want to update with it, is where the color is coming from. I think ideally, the code for determining the selected colors should be using a semantic token for the text and background color, but I'm not sure what the correct approach would be. There isn't a great example from Chakra's built-in components either, as they don't have a color highlighted version of their I'm happy to look into this some more when I have time, but I don't want to make any hasty decisions, because it would be technically a breaking change for anyone whose relying on the color being blue. Which is the main reason I left it as blue in v6 originally. |
Open in Stackblitz • chakra-react-select-demo
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind, I actually just ended up pushing my change requests to your branch directly, as I'd prefer to get this change out with the other changes I made. Thanks again for the PR!
And I'm still planning to do some thinking around the default color palette for the selectedOptionColorPalette
prop, as I think you may have a good point that it would make more sense to just use the user's default.
Currently properties about color palette are referencing directly type
ColorPalette
from@chakra-ui/react
.As of chakra ui does with v3, we should allow any string or custom colors won't work.