fix(sx): support Primer colors for border*Color properties
#3619
Conversation
🦋 Changeset detectedLatest commit: 968bae7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
||
| Include `border*Color` properties in sx to support named Primer colors | ||
|
|
||
| <!-- Changed components: _none_ --> |
There was a problem hiding this comment.
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 CLI
There was a problem hiding this comment.
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!
size-limit report 📦
|
|
|
||
| Include `border*Color` properties in sx to support named Primer colors | ||
|
|
||
| <!-- Changed components: _none_ --> |
There was a problem hiding this comment.
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!
I think If user feedback results in too much noise for their filtered changelogs than we can revise and think about additional keywords |
| 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 tsd?
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 .types.test suffixes where we do type only tests
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
Closes #3618
Screenshots
Before
After
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.