Skip to content
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

Fixed Sx type: added support of predefined aliases #294

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

v-honcharenko
Copy link

@v-honcharenko v-honcharenko commented Jun 16, 2023

Hi @nandorojo

First of all, thank you for your work on this great project!

I found a typing issue with the Sx prop related to aliases. For now, only the aliases of theme-ui are allowed by TS (bg, m, mt, mr, mb, ml, mx, my, p, pt, pr, pb, pl, px, py, size), all other defined aliases (like h, w, br) are not exposed to Sx type, and TS will complain when you will try to use it, however aliasing will work.

This PR fixes this issue, allowing to use TS intellisense for predefined aliases.

@vercel
Copy link

vercel bot commented Jun 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
dripsy ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 16, 2023 1:11am

Copy link

@Frosty21 Frosty21 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only question i have is the replacing "width" as "w" i would go for "width" as its more descriptive

examples/example/App.tsx Show resolved Hide resolved
packages/dripsy/src/core/types-v2/sx.ts Show resolved Hide resolved
@nandorojo
Copy link
Owner

Hey, you can use the aliases field on the theme to inject your own aliases too. Would that suffice to solve this?

@v-honcharenko
Copy link
Author

@nandorojo the issue is related only to typings of built-in aliases. All of these aliases are valid in JS code, and even in TS code it will work, BUT, TypeScript will complain on it, because these types are missed (only types, not aliases). This PR fixes that.

| keyof ViewStyle
| keyof TextStyle
| keyof ImageStyle
| keyof Omit<Aliases, Object.SelectKeys<Aliases, NonStyleableSxProperties>>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the late review. This could just just use Pick right?

It's a bit tricky to review all this and know it's working haha TS is so hard to look at in that way

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nandorojo Do you mean to replace Object.SelectKeys<Aliases, NonStyleableSxProperties> with keyof Pick<Aliases, NonStyleableSxProperties>>?
This will not work, because Pick selects by keys, while Object.SelectKeys selects by values, which is required for the map of aliases.

As the alternative solution, the aliases object can be split into 2 ones: styleable and non-styleable, and then this line can be replaced with:

Suggested change
| keyof Omit<Aliases, Object.SelectKeys<Aliases, NonStyleableSxProperties>>
| keyof StyleableAliases

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.

None yet

3 participants