danawoodman / react-fontawesome

A React Font Awesome component.
https://www.npmjs.com/package/react-fontawesome
MIT License
667 stars 72 forks source link

Changed syntax to use ES6 class and package prop-types since the are both deprecated #41

Closed menefotto closed 7 years ago

corime commented 7 years ago

you use two different ways to define properties inside of the class (displayName and propTypes) and both will, as far as I know, not work out of the box without installing another babel plugin. You could define the displayName using the constructor. The static propTypes are best defined after the class definition like so FontAwesome.propTypes = {...}

menefotto commented 7 years ago

I followed your advice and applied corrections...

danawoodman commented 7 years ago

@wind85 thanks for the PR! @coremi thanks for offering your feedback! Looks good minus some minor feedback!

danawoodman commented 7 years ago

Also, you're going to need to commit the built files by running npm run dist and committing the changes

menefotto commented 7 years ago

Hi, ok tomorrow morning I will also sent you the files built by npm run dist ... Do you mind if I also update the readme as well, in particular the contributing section to reflect the need to commit the files build by npm run dist ...

danawoodman commented 7 years ago

@wind85 yes that would be great! Thanks!

danawoodman commented 7 years ago

Thanks @wind85 for the PR! Just a few minor change requests before I merge! 🍻

danawoodman commented 7 years ago

@wind85 also, looks like you'll have to rebase because of the merging of #40

danawoodman commented 7 years ago

@wind85 once you rebase we can merge this in!

menefotto commented 7 years ago

I am a bit of a git newbie, therefore I am not too familiar with the all rebase process. I added one more commit the fixes the last one, I am sorry about that.

danawoodman commented 7 years ago

@wind85 if you can make me a contributor to your fork I can do it for you 😄

menefotto commented 7 years ago

Honestly this is my first contribution 🤓, I just started to use git some time ago. Is that ok, or I must rebase?

danawoodman commented 7 years ago

@wind85 yeah, we have to rebase since the other pull request changed some files. If you make me a contributor I can do it on your branch for you 👍

danawoodman commented 7 years ago

No worries about being new to Git/Github, I was too once! Happy to help you get this merged 😄

menefotto commented 7 years ago

Alright then I will do it tomorrow...

lesnitsky commented 7 years ago

@danawoodman Hi! What is current status of this PR? Really looking forward to it 😏

danawoodman commented 7 years ago

@wind85 I need you to run npm run dist to build the compiled code. Once that is done, I can merge :)

cc @R1ZZU

danawoodman commented 7 years ago

@wind85 thanks again for your patience on this, I promise we will get this in soon!

menefotto commented 7 years ago

@danawoodman Hi, you need to change the lint configuration otherwise it doesn't build, with the current settings...

danawoodman commented 7 years ago

You can try just npm run build for now

On Apr 19, 2017, at 1:46 PM, wind85 notifications@github.com wrote:

@danawoodman Hi, you need to change the lint configuration otherwise it doesn't build, with the current settings...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

menefotto commented 7 years ago

Anything wrong with it?

danawoodman commented 7 years ago

Thanks! Will check this out tomorrow!

danawoodman commented 7 years ago

Thanks @wind85! This is now in release v1.6.1!