fix(types): add sx prop whenever className can be string #1308
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dcastil
reviewed
Nov 27, 2020
packages/core/src/jsx-namespace.ts
Outdated
| ? Omit<P, keyof SxProps> & SxProps | ||
| : Omit<P, keyof SxProps> | ||
| : Omit<P, keyof SxProps> | ||
| export type WithConditionalSxProp<P> = { className: string } extends P |
There was a problem hiding this comment.
I like this one much more than the previous solution. I don't know why Emotion did it that way.
There was a problem hiding this comment.
Sorry, Dany. Now it's even uglier. I forgot that components can have more than one prop while writing this, and that's how I got that nice one-liner :D
|
Okay, I got greedy, and there's going to be another change, what you can probably see in failed tests. I'm making |
|
I didn't even touch this 😢 LibraryManagedAttributes can literally wreak generic components inference :D Okay, this needs to be fixed on our side. I don't want to add "our jsx pragma is incompatible with react-hook-form" to the readme :D
|
|
cc @beerose |
beerose
approved these changes
Nov 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I'm not exactly sure if we have the best possible implementation, as we don't produce
classNameat runtime, we producecssand call Emotion'sjsx, so it seems that we should also use their types for correctness.cc @atanasster @dcastil
Relevant to #1307, but I'm not sure if it closes.
There are three changes here.
The change to
WithPoorAsPropallows to read props of@theme-ui/mdxStyledon type level. Previously, after passing throught ComponentProps, they became dicts ofunknown.The change to
WithConditionalSxPropallows to usesxwith all components acceptingclassNameof typestring, even if they also accept other values forclassName.