fix(sx): support Primer colors for border*Color properties
#3619
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@primer/react': patch | ||
| --- | ||
|
|
||
| Include `border*Color` properties in sx to support named Primer colors | ||
|
|
||
| <!-- Changed components: _none_ --> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,12 @@ | ||
| import css, {SystemCssProperties, SystemStyleObject} from '@styled-system/css' | ||
| import {ThemeColorPaths, ThemeShadowPaths} from './theme' | ||
| import {ColorProps, BorderColorProps, ShadowProps} from 'styled-system' | ||
| import merge from 'deepmerge' | ||
|
|
||
| export type BetterCssProperties = { | ||
| [K in keyof SystemCssProperties]: K extends keyof ColorProps | ||
| ? ThemeColorPaths | SystemCssProperties[K] | ||
| : K extends keyof BorderColorProps | ||
|
There was a problem hiding this comment. @gr2m I know this is late, but I wonder if there's some tests we can use to ensure these don't regress, or to ensure they're fully expressive of options that work, and don't include options that don't work? There was a problem hiding this comment. this is a type-only change. Do you write tests for these with something like There was a problem hiding this comment. Also I feel if we added tests, we would only test behavior of styled-system? There was a problem hiding this comment. there are some other type-tests in the repo. I think since we're exposing a type that used in many places, it would be good to have some way to validate it, and ensure it's inclusive of all possible options. I don't think tsd is used in the repo, but might be valuable to add something like it for type only assertions? right now I think most type tests just ensure functions compile correctly - there's a bunch of files with There was a problem hiding this comment. okay cheers I'll send a follow up pull request, but can't say when I get to it |
||
| ? ThemeColorPaths | SystemCssProperties[K] | ||
| : K extends keyof ShadowProps | ||
| ? ThemeShadowPaths | SystemCssProperties[K] | ||
|
|
||
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.
In this case I think
_all_might have been the better option. If you agree and see that happening frequently let me know and I'll add the option to the changesets CLIThere 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.
I had a few PRs that I thought "all" would make more sense but some were a bit vague 🤔 Similar to this case like it does affect almost all components but it doesn't really feel a component change to me maybe more of a system change that can be communicated with a different keyword? Curious to hear what you think!