danawoodman / react-fontawesome

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

Please update to Font-Awesome 5 #69

Closed tashburn closed 6 years ago

santosguilherme commented 6 years ago

+1

danawoodman commented 6 years ago

Please use the official font awesome React component for v5.

https://github.com/FortAwesome/react-fontawesome

Reopen if something in React-fontawesome exists that the official component misses.

KagamiChan commented 6 years ago

Our project is using this package everywhere and migration will be too time consuming to take place for now. So I'm working on a compatible version, including:

@danawoodman What do you think?

danawoodman commented 6 years ago

@KagamiChan I'm always open to PRs. How do you plan on detecting FontAwesome v5 versus older versions? It would break if you changed from fa to fas, for example for all existing users of react-fontawesome.

One less than ideal option would be to include fa as well as fas|far|fal|fab eg fa fas fa-rocket.

Can you let me know how you're planning on approaching this before you jump off and work on it?

KagamiChan commented 6 years ago

Good question, my intention was to upgrade css to v5 with existing code unchanged.

v5 is divided into different sets and many icon name has changed, which introduced name collisions. So if we're planning to support both v4 and v5 css, without a flag the result could be unexpected.

Example: glass in v4 has been renamed to glass-martini <FA name="glass" /> could render:

<span class="fa fas fa-glass fa-glass-martini" />

for v4 css, fa and fa-glass will take effect, and for v5 css, fa, fas and fa-glass-martini will match the selectors. That's OK.

However, google-plus in v4 is moved to fab and renamed to google-plus-g in the same way <FA name="google-plus" /> could render:

<span class="fa fab fa-google-plus fa-google-plus-g" />

However in v5 fab provides another google-plus icon, the result is a little tricky.

So my proposal is:

So the result for above example is:

Usage Rendered
<FA name="glass" /> <span class="fa fas fa-glass-martini" />
<FA v4 name="glass" /> <span class="fa fa-glass" />
<FA fas name="glass" /> <span class="fa fas fa-glass" /> (no matched result)
<FA fab name="glass" /> <span class="fa fab fa-glass" /> (no matched result)
<FA name="google-plus" /> <span class="fa fab fa-google-plus-g" />
<FA v4 name="google-plus" /> <span class="fa fa-google-plus" />
<FA fab name="google-plus" /> <span class="fa fab fa-google-plus" />
<FA fas name="google-plus" /> <span class="fa fas fa-google-plus" /> (no match result)
danawoodman commented 6 years ago

@KagamiChan thanks for putting some thought into this!

My main concern is breaking things for consumers of v4 since that is literally all react-fontawesome's users. Since FontAwesome has an official component that supports v5, I would encourage people to transition over to their component. Perhaps a "find and replace" is in order in your codebase?

That or create a intermediary component like:

export default function Icon({ name, ...props }) {
  return <FA name={name} {...props} />
}

And then you can just migrate to using this <Icon> component in your codebase and choose which implementation of the FontAwesome component you want to use.

Since the icon names change and muddying up the class names is sort of a hack, I think the best plan is for people wanting to use v5 is to just move to the official plugin (via find/replace or maybe a codemod?).

I know this causes you issues but the alternative is to break this component for every user which right now I don't find to be the best option. Hope you understand!

danawoodman commented 6 years ago

Going to close for now. Feel free to convince me I'm wrong though! 😄

KagamiChan commented 6 years ago

Really appreciate your detail explanation and I understand your concerns.

I'll try to make out a less painful way to migrate with official packages, and append my findings if they worth noting down.

twig-openlearning commented 6 years ago

@danawoodman thanks for the informative responses. would be a good idea to put that in the README to avoid more of these issues being created

danawoodman commented 6 years ago

@KagamiChan @twig-openlearning I have added a note about using the official component in the readme 🕺