-
Notifications
You must be signed in to change notification settings - Fork 265
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
Adding types, derived from fontawesome API types #46
Conversation
This an alternative proposal to PR #33 , but is inspired by the initial work done there by @Aidurber, @PeterKottas, and @mboudreau. |
I'm excited to take advantage of react-fontawesome using typescript... is there anything I can do to help get this change in? |
Hi @NateRadebaugh. I'm in the midst of finalizing some major work to support TypeScript usages, and expect to release it today or Monday. I think we've got all the resources needed to get that done soon. But when it's out, some feedback after you start using would be great. |
… checked in. - add a gitignore rule to avoid checking it in the future. make sure that ignore rule doesn't impact index.js files in subdirectories.
src/index.js
Outdated
import fontawesome from '@fortawesome/fontawesome' | ||
import FontAwesomeIcon from './components/FontAwesomeIcon' | ||
import { noAuto } from '@fortawesome/fontawesome' | ||
export { FontAwesomeIcon } from './components/FontAwesomeIcon' |
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.
Can we make this export default as FontAwesomeIcon from './components/FontAwesomeIcon'
?
src/components/FontAwesomeIcon.js
Outdated
@@ -45,23 +45,23 @@ function normalizeIconArgs (icon) { | |||
} | |||
} | |||
|
|||
function FontAwesomeIcon (props) { | |||
export function FontAwesomeIcon (props) { |
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.
My only complaint with this is that I would consider src/components/FontAwesomeIcon.js
an internal component. So I would expect there to be a default export in this file. It feels strange to need to do this in the test:
import { FontAwesomeIcon } from '../FontAwesomeIcon'
Can we switch this to export default function (props) {
@robmadole just pushed changes. Do they satisfy your requests? |
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.
Perfect-o!
@mlwilkerson yes, thank you. This looks good-to-go 👍. |
UPDATE: A pre-release version with these changes has been published as |
* Add TypeScript types, dependent upon the types in the fontawesome API package. * Add example app that uses TypeScript * Add @types/react package to dev dependencies * Reformat with prettier * Moving from default imports to named imports, both in how this component uses the underlying fontawesome API, and in what this component exports to its clients. * Update documentation to remove default export/import.
* Add TypeScript types, dependent upon the types in the fontawesome API package. * Add example app that uses TypeScript * Add @types/react package to dev dependencies * Reformat with prettier * Moving from default imports to named imports, both in how this component uses the underlying fontawesome API, and in what this component exports to its clients. * Update documentation to remove default export/import.
What about a final release with this? |
We'll get his released when we feel like it's been tested and matured enough. We aren't there yet. |
Can you release a pre-release which adds |
I forked and added it: https://github.com/kirkbross/react-fontawesome/tree/f-kirkbross-color-prop |
* Add TypeScript types, dependent upon the types in the fontawesome API package. * Add example app that uses TypeScript * Add @types/react package to dev dependencies * Reformat with prettier * Moving from default imports to named imports, both in how this component uses the underlying fontawesome API, and in what this component exports to its clients. * Update documentation to remove default export/import.
* Add TypeScript types, dependent upon the types in the fontawesome API package. * Add example app that uses TypeScript * Add @types/react package to dev dependencies * Reformat with prettier * Moving from default imports to named imports, both in how this component uses the underlying fontawesome API, and in what this component exports to its clients. * Update documentation to remove default export/import.
* Add TypeScript types, dependent upon the types in the fontawesome API package. * Add example app that uses TypeScript * Add @types/react package to dev dependencies * Reformat with prettier * Moving from default imports to named imports, both in how this component uses the underlying fontawesome API, and in what this component exports to its clients. * Update documentation to remove default export/import.
This PR depends upon the type definitions being added to the underlying
@fortawesome/fontawesome
and@fortawesome/fontawesome-free-solid
npm packages.So, for those who may like to see how these work together, I've also attached tarballs with alpha versions of
@fortawesome/fontawesome
and@fortawesome/fontawesome-free-solid
with type definitions.For any reviewers who'd like to set this up to try it out (like with the example app
typescript
app I've added to this repo), I recommend that you simply use the attached tarballs as dependencies ofreact-fontawesome
vianpm link
oryarn link
.fortawesome-fontawesome-1.1.1-alpha3.tar.gz
fortawesome-fontawesome-free-solid-5.0.3-alpha2.tar.gz
[UPDATE 12-29-17: updated alpha tarballs and removed type def listings from this comment, because they got much longer after adding all of the icon names. But you'll find them in the tarballs.]