Uh oh!
There was an error while loading. Please reload this page.
Adds theme keypaths to our sx prop type for better autocomplete - #1544
Conversation
🦋 Changeset detectedLatest commit: 82a4a68 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 |
size-limit report 📦
|
fd8ec49 to
5de709fCompare
colebemis
left a comment
There was a problem hiding this comment.
This is an excellent incremental step towards better TypeScript support for the sx prop. No need to over-engineer a solution until we solidify the future of CSS-in-JS in Primer React. Love it 💖
| subtle: string | ||
| } | ||
| // Only meant for Primer components |
There was a problem hiding this comment.
I'm not sure if we want autocomplete for primer.* variables. Let's leave them out for now.
| export type ThemeShadowPaths = KeyPaths<ThemeShadows> | ||
| // Produces a union of dot-delimited keypaths to the string values in a nested object: | ||
| type KeyPaths<O extends {}> = { |
There was a problem hiding this comment.
Should this live in src/utils/types/?
5de709f to
3095551Compare3095551 to
bacbdf0Compare| @@ -0,0 +1,5 @@ | |||
| --- | |||
| '@primer/components': patch | |||
There was a problem hiding this comment.
I'm on the fence about whether this is a minor or patch release. Do we consider the lack of types a bug?
There was a problem hiding this comment.
Hmm, good question. Maybe it's a minor? It's sort of a new feature, but also the API hasn't really changed at all? Even the types should be functionally equivalent — anything that passed typechecking before passes now, and anything that didn't pass before fails now.
There was a problem hiding this comment.
(TLDR: no strong feelings, happy to switch to minor if you think that's more appropriate)
| } | ||
| } | ||
| export function generatesKeyPathsFromObject(x: KeyPaths<NestedObject>): 'a' | 'b.b1' | 'b.b2' { |
There was a problem hiding this comment.
Thanks again for setting these up!
siddharthkp
commented
Oct 29, 2021
Exciting!!! 🎉 |
This replaces the type currently used for the
sxprop with one that explicitly includes all key paths into our theme for color and shadow values. This does not yet add better typing to styled system props.Partially addresses https://github.com/github/primer/issues/389
Open questions
ColorPropsandShadowPropsobjects an acceptable proxy for getting a list of keys intoSystemCssPropertiesobjects that have color and shadow variables? In theory styled system prop names could diverge fromSystemCssPropertiesproperties' names over time.Screenshots
CleanShot.2021-10-27.at.15.45.40.mp4
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.