search

danawoodman / react-fontawesome

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

Add property to allow changing type of containing component aside from span. #19

Closed n3v3rf411 closed 7 years ago

danawoodman commented 8 years ago

Hi @n3v3rf411 thanks for the PR. What is the intention/goal of making this change? From what I can tell, <span> is the preferred tag here since <i> is semantically meant to indicate italic text. I know people sometimes prefer <i> but what is the practical reason for such a change other than preference?

hassankhan commented 8 years ago

Hi, <span> may be the preferred tag but <i> is also used a LOT. In my particular case, I need to be able to customise the tag to match a vendor's CSS so I can style things correctly.

danawoodman commented 8 years ago

@hassankhan so the vendor is using the tag name instead of class name to select the icon? Damn vendors! 😉

I'd be open to merging this if we could address the two things I mentioned in the commit!

hassankhan commented 8 years ago

I'm ... actually not the author of the PR, I do agree with you on changing component to tagName and adding tests for it.

n3v3rf411 commented 8 years ago

Oooops I totally forgot about this PR. I have the same reasons with @hassankhan. We are using AdminLTE and it uses <i> alot for fontawesome icons. Most CSS selectors esp. the sidemenu use element>element selectors.

I will apply the changes above and resubmit the PR tomorrow.

danawoodman commented 8 years ago

Lol sorry @hassankhan! @n3v3rf411 could you make those small tweaks and I'll merge? Thanks!! 🍻

danawoodman commented 7 years ago

@n3v3rf411 if you can make those tweaks above I will merge 👍

danawoodman commented 7 years ago

@n3v3rf411 let me know if you can make those changes :)

danawoodman commented 7 years ago

Closing this until PR is updated