Skip to content
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

Upgrading to 5.1.0 #100

Merged
merged 13 commits into from
Mar 28, 2018
Merged

Upgrading to 5.1.0 #100

merged 13 commits into from
Mar 28, 2018

Conversation

robmadole
Copy link
Member

No description provided.

@robmadole robmadole requested a review from mlwilkerson March 19, 2018 16:54
UPGRADING.md Outdated
@@ -61,6 +61,12 @@ import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
import library.add(fas, faTwitter)
```

This is also a valid way to import icons that works if your tool does not support tree shaking:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great

optional, so we've omitted them.)
* Or it could be an `Array` of strings, where the first element is a
prefix, and the second element is the icon name: `{["fab", "apple"]}`
### Processing `<i>` tags into `<svg>` using Font Awesome
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be helpful to state more specifically in this section that processing <i> tags is not the React Way. It's really the sans-React way. And the reason for mentioning that here is to say that, if you know that you want to be able to do both (a mixed mode), here's how you can enable that.

When I try to imagine myself in the shoes of a newbie reading this, it might make me think: "OK, this section is tell me that if I want basic Font Awesome support, I need to enable it with watch()."

What I'd want her to understand is more like this:

  • if what you really want to do is use <i> and you're not interested in React, then you don't need this React component, just go use the basic SVG with JS method and <i> tags.
  • if what you really want to do is use React, then you never need to use <i> tags. And because we assume that you're using the React component because you want to use Font Awesome in a way that optimized for React, we've disabled the <i> tag replacement stuff by default as an optimization.
  • but if you know that you need to have both things going on at the same time, then this is how you'd enable the <i> tag autoReplace, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice point. Ok, let me take another shot at that.

Copy link
Member

@mlwilkerson mlwilkerson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

Just the one set of suggestions to consider in the README.md, but not blocking.

README.md Outdated
Our hope and intention is that React users will use this package (`react-fontawesome`)
when using Font Awesome. This component leverages React's architecture and philosophy.

However, **if you cannot use these components everywhere in your app and still
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, great. That clears it up for me and my imagined former newbie self.

@robmadole
Copy link
Member Author

Thanks, @mlwilkerson !

@robmadole robmadole merged commit b92d569 into development Mar 28, 2018
@robmadole robmadole deleted the upgrading-to-5.1.0 branch March 28, 2018 19:27
robmadole added a commit that referenced this pull request Mar 28, 2018
@KagamiChan
Copy link

@robmadole Seems you're adding babel-preset-env to package dependency, is this intended?

@robmadole
Copy link
Member Author

Nice catch! Fixed it now in development and released 0.1.0-8. Thank you!

robmadole added a commit that referenced this pull request May 15, 2018
robmadole added a commit that referenced this pull request Jun 20, 2018
robmadole added a commit that referenced this pull request Jun 20, 2018
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.

3 participants