-
-
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
Update typescript document to match with current types #679
Conversation
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.
Thanks for this!!
Could you move the create-emotion and create-emotion-styled parts to the bottom and move the parts about type checking objects into the emotion section since the vast majority of users will only use emotion and react-emotion so they shouldn't have to care about create-emotion and create-emotion-styled?
docs/typescript.md
Outdated
`; | ||
``` | ||
|
||
Typescript checks css properties in object style syntax using csstype package, so following code will emit errors. |
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.
Could you remove the bit about using csstype
since it's an implementation detail and people don't need to know about it.
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.
But it would be helpful when users meet some type issues.... wouldn't it?
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.
Yeah, makes sense, could you make it a link to the project as well then?
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.
Oh... you're right. I will add the link too.
docs/typescript.md
Outdated
|
||
## create-emotion-styled | ||
|
||
Current typing for `create-emotion-styled` is only compatible with React, and will not work with Preact. For detail typing, see following `react-element` section. |
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.
Could you change "Current typing for create-emotion-styled
is only" to "The current typings for create-emotion-styled
are only"?
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.
OK. Thank you for your review!
docs/typescript.md
Outdated
@@ -2,9 +2,77 @@ | |||
title: "Typescript" | |||
--- | |||
|
|||
Emotion includes TypeScript definitions for `styled` components and has type inferences for both html elements and React components. | |||
Emotion includes TypeScript definitions for `create-emotion`, `emotion`, `create-emotion-styled`, and `react-emotion` packages. These definition also could infer types for css property (with object syntax), and HTML/SVG tag name, and props types. |
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.
Is there a circumstance where it can't infer types for css properties?
If not, could you replace "These definition also could infer types for css property" with "These definitions also infer types for css properties"?
Also, could you replace "HTML/SVG tag name" with "HTML/SVG tag names"?
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.
Allright. I will change them.
ef53d27
to
d02f84e
Compare
docs/typescript.md
Outdated
|
||
## preact-emotion | ||
|
||
Types are almost same with `react-emotion` package. |
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.
If they're almost the same and not the exact same, could you explain how they're different?
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.
OK. I will add following.
However, this typing use Preact component types like
ComponentConstructor
andFunctionalComponent
instead of React component types.
Is this enough?
docs/typescript.md
Outdated
|
||
## create-emotion-styled | ||
|
||
The current typings for `create-emotion-styled` are only compatible with React, and will not work with Preact. For detail typing, see `react-element` section above. |
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 isn't true anymore about not supporting Preact, right?
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.
No, it still does not work with Preact, since we only have preact-emotion
typing with Preact, and does not have create-emotion-styled
typing with Preact.
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.
FYI, Preact and React both modifies JSX.Element
, so we cannot import both typings in one file. That's why I did not provide typing for preact version of create-emotion-styled
. (Its index typing file already import React typing)
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.
The only way I can image, to deal with the problem, is, providing index typing without import any (P)React related typings and making users import them.
However, this may confuse users, because it will not work properly when users do not import (P)React related typings.
docs/typescript.md
Outdated
### Define a Theme | ||
|
||
By default, the `props.theme` has `any` type annotation and works without error.\ | ||
However, you can define a theme type by creating a another `styled` instance. | ||
|
||
_styled.tsx_ | ||
_styled.jsx_ |
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.
Why the change in extension, isn't tsx
generally used?
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.
Oh, my mistake. Sorry. I will change these.
const Image = styled<ImageProps, 'div'>('div')` | ||
background: url(${props => props.src}) center center; | ||
const Image0 = styled('div')` | ||
width: ${(props: ImageProps) => props.width}; |
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.
Why are you using width: ${(props: ImageProps)
instead of typing the component with styled<ImageProps>('div')
? This is a bit confusing for me, since I see the latter as a much clearer way to type your component.
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.
Because in (will-be-)updated typings, there're no type parameters Props for styled
.
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.
In TS 2.9 (which is in RC), you can use template string generic, however, before it, you should give a type to function argument and let TS infer correct type for component.
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.
Why? Typing every prop function with ${(props: ImageProps)
is more work, right? The redundant div
in styled<ImageProps, 'div'>
is a bit frustrating, but it will not be necessary anymore as soon as TypeScript 2.9 is released.
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.
In 2.9, you also don't need to type every function with new typings, since, as I said, you can use template string generic.
Before 2.9 with old typings, you should type <..., 'div'>, and sometimes even more complex things in second type parameter, and it could make really hard to type complex and large app.
The problem what you mentioned occurs only when you use more than two function arguments, and as far as I experienced, it's not that common... (Of course, this is totally my subjective opinion)
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.
what you mentioned occurs only when you use more than two function argument, and as far as I experienced, it's not that common
In my experience I have a lot of these functions, if you are building a component library you can easily have ten of those prop functions. But if this isn't relevant anymore with TS 2.9 I digress.
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.
Maybe my opinion is from my object syntax based experience. Even when I made a library, I rarely used more than two function arguments.
(Just one or two functions those return proper object styles)
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.
And, thank you for your feedback!
Hi folks, I just hit an issue. I have a button component that uses custom props but fallsback to theme props. But if I use (props: ButtonProps) => whatever I lose typing to Themed. I've been trying to get this to work and failing. Do you have any pointers? Should I make something Themed for this cases? Maybe it's a good thing to add in the docs |
@felipeleusin If you use TS >= 2.9, you can use something like styled('div')<ButtonProps>`
${props => something}
`; |
Oh, ok! Will try this. Thanks a lot! |
I should add that to the document... |
I can add a PR for that, was just checking it's working first :) |
@felipeleusin Oh, I'm not start yet. Currently I'm discussing about typing in #721. |
What: Update typescript document
Why: Typings are changed, so document is outdated.
Checklist:
This document depends on #678 and #680, so please do not merge this before them.