Added typescript definitions #51
Conversation
There was a problem hiding this comment.
Hi @ephys, I've given some feedback on this PR. I think my first comment is probably the most important - have you written declaration files for modules before? I haven't, so I don't know the correct format - if you have before, ignore my comment.
| @@ -0,0 +1,29 @@ | |||
| declare module 'storybook-addon-knobs' { | |||
| import WrapStory from "../src/components/WrapStory.js"; | |||
There was a problem hiding this comment.
How does this work @ephys? WrapStory has no declaration file, so wouldn't this throw an error?
| story: string, | ||
| } | ||
|
|
||
| export function knob<T>(name: string, options: KnobOption<T>): T; |
| @@ -0,0 +1,29 @@ | |||
| declare module 'storybook-addon-knobs' { | |||
There was a problem hiding this comment.
Should this be @kadira/storybook-addon-knobs?
There was a problem hiding this comment.
I'm not actually sure how declaration files work when included with modules.
Using redux definitions as reference:
https://github.com/reactjs/redux/blob/master/index.d.ts
Seems like we don't need the declare module wrapper?
|
Thanks for the PR by the way @ephys |
|
|
||
| export function knob<T>(name: string, options: KnobOption<T>): T; | ||
|
|
||
| export function text(name: string, value: string): string; |
There was a problem hiding this comment.
We also support null for the value. Is that something supported by TS.
There was a problem hiding this comment.
Yes, this can be value: string | null.
If you want it to be optional as well, the syntax is value?: string | null
|
@frederickfogerty Thanks for reviewing this. Could you ping me when it's ready to merge? |
|
@frederickfogerty Yep I got it. |
|
|
||
| export function select<T>(name: string, options: { [s: string]: T }, value: string): T; | ||
|
|
||
| export function date(name: string, value: Date = new Date(0)); |
There was a problem hiding this comment.
value: Date = new Date(0) is causing an error when I try to compile this. Parameter initializers are not needed in declarations, this can just be value?: Date.
|
@arunoda For sure! If I happen to miss any updates, please ping me and I'll give it another review 😄 |
|
Thanks for the review! I'll include the changes this afternoon (gmt) :) |
|
Hi! |
|
More sick than busy, unfortunately the result is the same. :( On hold until I get better |
|
@ephys ok, wish you getting well soon |
|
Thank you! |
|
Get well soon 😄 @ephys |
|
@frederickfogerty Not sure if that is possible in Github. But I think you can easily submit another PR with work on top of this commit.
|
|
@roonyh thanks, I just wanted to give as much credit as possible to @ephys, so I wanted to work on this PR instead. If he doesn't get back to me, I'll pull his fork. And by the way, the steps should add his fork as a separate remote, and then pull from that.. not sure if pulling directly from a git repo would work (I'm willing to give it a try though) |
|
@frederickfogerty Ephys' commit will be preserved with his info so he definitely will be credited for his work :) It does not matter who make the PR much, as long as the author field carries correct details in each commit. So don't worry about that. I think I have successfully pulled from urls like that. Give it a try :) |
Would these match your needs for issue #33 ?
Don't hesitate to point out anything I might have done wrong or missed! :)