-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Enable css prop in Preact with TypeScript #729
Conversation
@aaronjensen Thank you for your PR! Could you add tests for |
21e63b4
to
aafd3b5
Compare
@Ailrun done |
aafd3b5
to
75b629d
Compare
@Ailrun added a fix for it as well, let me know if that works or if you'd like something different. Thanks! |
@@ -23,7 +23,7 @@ export type Themed<P extends object, T extends object> = P & { theme: T }; | |||
|
|||
export type StyledStatelessProps<P extends object, T extends object> = | |||
& P | |||
& { theme?: T } | |||
& { theme?: T, css?: CSSObject | string } | |||
; |
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.
This will only work with StyledComponent. As far as I understand, you can use css
props in any components. You can see how I added css
props in
emotion/packages/emotion/types/index.d.ts
Lines 19 to 23 in a8f4f89
declare module 'react' { | |
interface HTMLAttributes<T> { | |
css?: Interpolation; | |
} | |
} |
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.
Or does it work differently with preact-emotion/babel-plugin-emotion?
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.
Ah, ok, thanks. I'll try that w/ preact. Should I leave the above for StyledComponent
as well?
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.
By the way, why is that in emotion/types
rather than react-emotion/types
? Should I put the preact one in emotion/types
as well? Is the idea that you could theoretically use babel-plugin-emotion
w/o preact-emotion
?
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.
@aaronjensen I think so... since some one should be able to use css
props with emotion
(without preact-emotion
), and if emotion
already provides such typings, providing it again sounds like going against DRY.
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.
@aaronjensen Oh, and as an answer to your question, as you can see in the https://emotion.sh/docs/css, it does not require anything but emotion
and babel-plugin-emotion
.
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.
@Ailrun updated
75b629d
to
3e64415
Compare
@@ -23,7 +23,7 @@ export type Themed<P extends object, T extends object> = P & { theme: T }; | |||
|
|||
export type StyledStatelessProps<P extends object, T extends object> = | |||
& P | |||
& { theme?: T } | |||
& { theme?: T, css?: BaseInterpolation } |
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.
This shouldn’t be here since the css prop isn’t specific to styled components
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.
I can remove it, but then the tests in create-emotion-styled
fail unless I add import 'emotion';
. Would you rather that or that I remove the tests?
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.
Updated w/ the tests in place, let me know if you want me to remove them.
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.
How about moving those HTMLAttributes
into create-emotion
?
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.
Done, as a separate commit. Let me know if you want me to squash them.
3e64415
to
33a5f0d
Compare
What:
This adds typescript tests for the css prop for both React and Preact. The
Preact tests are currently failing, so they'd need to be fixed before this can
be merged. See #693 (comment)
Why:
The tests weren't there before and the css prop only worked with React.
How:
I'm not sure how to answer this question.
Checklist: