Skip to content

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

Merged
merged 3 commits into from
Mar 25, 2025

Conversation

saalto-it
Copy link
Contributor

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.

image

Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@saalto-it
Copy link
Contributor Author

saalto-it commented Mar 22, 2025

Also, as official documentation states https://www.chakra-ui.com/guides/theming-change-default-color-palette, the default colorPalette is gray.
I think we should use that instead of the current blue used by the library, don't we?

@csandman
Copy link
Owner

csandman commented Mar 25, 2025

Hey thanks for the PR! First, I just want to point out that you can use custom colorPalette options if you have properly generated your types with @chakra-ui/cli typegen. I tested that out and have found it to work in my demo project. I tested it out just following this guide: https://www.chakra-ui.com/guides/theming-custom-colors

However, I am thinking you're right that the colorPalette prop should be less restrictive and should match Chakra's built-in behavior, but this PR doesn't fully cover it. I took a look at the typing of the colorPalette prop from what you linked, and am seeing the following:

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 UtilityValues["colorPalette"] type is the same as the exported ColorPalette type that Chakra exposes, so that part of what you had is fine. Especially considering the UtilityValues prop isn't actually exported (this is a big pain point with figuring out what types map exactly).

Now the one big problem with your ConditionalValue<ColorPalette | string> approach is that it doesn't allow for autocompletion in an editor. The whole purpose of Chakra's AnyString type is that it allows to do a union with a string type while still allowing for autocompletion with other constant string types. For example, this is what it's supposed to look like:

image

And this is what it looks like with your version:

image

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 types.ts file:

export type CssVars = `var(--${string})`;

export type AnyString = string & {};

export type ColorPaletteProp = ConditionalValue<
  ColorPalette | CssVars | AnyString
>;

Unfortunately, the CssVars and AnyString types aren't exported from Chakra, so they'd have to be redefined. Not a huge deal though.

Then change the types of all of the colorPalette props to be ColorPaletteProp. Then finally, you'll have to update the typing on realSelectedOptionColorPalette in the useChakraSelectProps hook to be ColorPaletteProp as well in order to fix the typings that are broken there.

If you make these changes, I'd be happy to merge!


EDIT: There are actually a couple more instances where ColorPalette would need to be replaced with the new ColorPaletteProp type in the multi-value.tsx file. Make sure to do a lint after making the changes.

@csandman
Copy link
Owner

Also, as official documentation states chakra-ui.com/guides/theming-change-default-color-palette, the default colorPalette is gray.
I think we should use that instead of the current blue used by the library, don't we?

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 Select component, only a check.

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.

Copy link

pkg-pr-new bot commented Mar 25, 2025

Open in Stackblitzchakra-react-select-demo

npm i https://pkg.pr.new/chakra-react-select@362

commit: eb912ea

@csandman csandman self-requested a review March 25, 2025 16:16
@csandman csandman self-assigned this Mar 25, 2025
Copy link
Owner

@csandman csandman left a 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.

@csandman csandman merged commit 1611b50 into csandman:main Mar 25, 2025
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants