-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
First try at adding missing type annotations. #84
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
Conversation
|
I can't understand why the travis checks have failed. I've modified only |
|
@NulledGravity not sure what's travises problem, but it's not your fault. I get the same error, though the local build seems to be fine. Is your PR complete? |
|
@drcmda I've added the types following https://github.com/drcmda/react-spring/blob/master/API.md. I'm relatively new in TS and haven't had the chance to test if all the types are correct. I'd like to get some advice/help on how to complete the annotations. Also I still need to comment all the types. Any help welcome :) |
|
Great! Could you make a last check and see if it corresponds to the actual proptypes in the js files? API.md could be slightly outdated since there were some changes recently, for instance to the way config and immediate work, they can be functions now that return individual results. If you think the PR is done i'll merge - i myself have zero knowledge sadly when it comes to TS. |
| */ | ||
| items?: Array<TransitionItemProps> | TransitionItemProps; | ||
|
|
||
| children?: ( |
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.
Transition also accepts false as a valid value for the child (in case you are tracking a single child that you do not wish to render), so this should be
+ children?: ((
+ params: DS & S,
+ ) => ReactNode | Array<(params: DS & S) => ReactNode>) | false;|
|
||
| export class Transition< | ||
| S extends object, | ||
| DS extends object, |
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'm getting TS1009 no trailing commas allowed on the latest version of TS, could you please remove them?
|
@jariz so, you're saying it should go in in this shape? |
|
@drcmda yep. only spotted this because I was using it in my exact usecase, but def gonna need some tests at some point down the road to double check if they're all correct. |
First try at adding missing type annotations.
I don't have much experience with TS, haven't had the chance to test the annotations, let me know if there's something that doesn't work.