Skip to content

Add TypeScript Definition #29

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

Merged
merged 2 commits into from
Nov 28, 2018
Merged

Conversation

Saturate
Copy link

Add a TypeScript Definition, and "build" it with webpack to the build folder.

This will add correct typings, if you use TypeScript with React.

#27

Add a TypeScript Definition, and "build" it with webpack to the build folder.

This will add correct typings, if you use TypeScript with React.

Mastermindzh#27
@Mastermindzh
Copy link
Owner

Mastermindzh commented Nov 27, 2018

Hey man, thanks for taking the time to develop this.

I got a few things I'd like to change before we merge though.

  • Consent is written with an "s", you wrote concent instead, so I'd like to change it to use the S instead.
  • The Javascript file uses 2 spaces for indentation, we should keep that the same for TS I reckon (though I doubt we need a tslint config).
  • Please remove the I prefix on the interface (as per the typescript handbook)

Again, much-appreciated man! This is some solid work 👍
Fix these few small things and I'll be sure to merge it in ASAP.

@Saturate
Copy link
Author

Oh, went a little too quick there it seems.
Thanks for pointing it out :-) will fix it soon.

Last commit went a little too fast it seems, this fixes:

- Spelling error
- Indentation
- Code Syntax (No interface prefix)
@Saturate
Copy link
Author

Should now be fixed, out of curiosity can you point me to the point where they say no prefix for interfaces? I think tslint wants this by default.

@Mastermindzh
Copy link
Owner

Hey @Saturate,

The changes look good!

It seems they changed the handbook a while ago, I wasn't aware of that change.
You can apparently use both now. The React Typescript starter by Microsoft (those who wrote the handbook) by default doesn't care whether you prefix or not.

Google is still full of discussions about this specific point, some interesting points of view on there too :)

So... that change wasn't necessary after all :)
I'll merge it like this, thanks for the hard work mate!

I'll push an update immediatelly after the merge too (it will be a minor one)

@Mastermindzh Mastermindzh merged commit 04c553f into Mastermindzh:master Nov 28, 2018
@Saturate
Copy link
Author

Hey @Mastermindzh

Thanks for the update and if you ever visit Copenhagen, I'll (well the company) give you a beer ;)

@Mastermindzh
Copy link
Owner

If you ever visit the Netherlands I'll return that favor man!

Great working with ya!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants