-
-
Notifications
You must be signed in to change notification settings - Fork 106
Fix IntelliSense override in Space type by refining Pixel #428
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
base: main
Are you sure you want to change the base?
Conversation
@quantizor Do you think that's something we can merge? |
I don't have sufficient knowledge of TypeScript for this, and I'm not currently in the business of merging xstyled PR's anyway, but I have one request: This sort of non-obvious code should always include internal documentation to explain it and preferably point to reference documentation. |
@agriffis Thanks for the note! I’ll add a comment above the type definition explaining the reasoning and linking to that issue. Curious — do you have a preferred way of documenting this kind of subtle or non-obvious type behavior? Always happy to align with your expectations. |
Internal documentation seems to be a lost art in the modern era... Something like this: // This syntax enables Intellisense to prioritize theme tokens over raw values.
// See https://github.com/microsoft/TypeScript/issues/29729 |
Haha, it really is. I keep hearing “your code should document itself!” — but sometimes it needs a little help. Just pushed a clear inline comment to document the intention. Thanks again for the suggestion! |
@agriffis For better reference, here’s the IntelliSense improvement in action: It's a super minimal change, but it makes a big impact on developer experience. |
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.
LGTM
@quantizor I contributed a review and some comments here, but I won't step on your toes by merging it directly. Assume you'll do that when you're ready. |
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.
+1
👋🏻 @gregberge WDYT? |
Hey, I am no longer maintaining xstyled. If you use it you should migrate to Tailwind CSS 😌 |
🛠️ Fix: Improve Type IntelliSense by Refining Pixel Definition
Issue:
The current Pixel type is defined as string | number, which causes IntelliSense to prioritize raw values over theme tokens (e.g., 'sm', 'md', etc.) when used in union types like Space = ThemeSpace | Pixel.
Solution:
Redefine Pixel as (string & {}) | (number & {}), which maintains compatibility with raw values at runtime but defers IntelliSense suggestions in favor of theme tokens.
This enables cleaner developer experience with proper token autocompletion while retaining full runtime flexibility.