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

Icon component PropType update to FontAwesome 4.7.0 #1371

Closed
brianespinosa opened this issue Feb 22, 2017 · 12 comments · Fixed by #1372
Closed

Icon component PropType update to FontAwesome 4.7.0 #1371

brianespinosa opened this issue Feb 22, 2017 · 12 comments · Fixed by #1372

Comments

@brianespinosa
Copy link
Member

It appears the PropType validation for name on the Icon component needs to get updated to support the new FontAwesome 4.7.0 icons that were just added in the most recent SUI release.

Steps

Set the name property to "address book outline" on an Icon component or on a icon property for a List.Item component

Expected Result

The "address book outline" icon should show and there should be no console error for an invalid string passed.

Actual Result

The icon is rendered correctly but there is a console error:

Warning: Failed prop type: Invalid prop `name` of value `address book outline` supplied to `Icon`.

Instead of `address book outline`, did you mean:
  - disk outline
  - bell outline
  - hdd outline

Version

0.65.0 SUIR
2.2.9 SUI

Testcase

http://codepen.io/anon/pen/gmYGXZ

@brianespinosa brianespinosa changed the title Icon component PropType update t0 FontAwesome 4.7.0 Icon component PropType update to FontAwesome 4.7.0 Feb 22, 2017
@brianespinosa
Copy link
Member Author

I just confirmed that this is also an issue in v0.66.0 of SUIR.

@brianespinosa
Copy link
Member Author

@layershifter I can see that these strings are coming from our SUI.js with a note saying the constant arrays were generated from the main icons CSS file in SUI proper. However... can't find anything that is automatically generating them. I assume this means those arrays were assembled manually then?

If there's no auto generating tool for this, I can take on this task and submit a PR on Friday.

@kamdz
Copy link
Contributor

kamdz commented Feb 22, 2017

I did it ✌️

@brianespinosa
Copy link
Member Author

Moving forward, I think SUIR might benefit from having a way to pass other string arrays to the Icon component outside of the current FontAwesome icon offering. There are other icon libraries out there and it would be great to extend this component with other names. The CSS will of course be handled outside of SUIR, but the PropType validation I think would still be important.

It seems to me that in the case of the Icon component we are coupled to those styles.

@levithomason
Copy link
Member

Regarding extending the icon names, see #931 and #955

@brianespinosa
Copy link
Member Author

Thanks for the quick response, @levithomason. I was thinking in terms of the string name validation... but at the end of the day your position on that in this comment makes the most sense for keeping 1:1 with SUI.

I'm already using className to extend the Icon component in a few areas where I have an additional font library.

Do you think there would be any value to updating the docs with a note about doing this for an external/additional icon font library? I could do that.

@kamdz
Copy link
Contributor

kamdz commented Feb 22, 2017

However, I thing that my pr with updated SUI icon names/aliases array is still good for this case, and for any other custom icon take @levithomason solution

@levithomason
Copy link
Member

Quick clarification, SUI does not use FA classes: #1372 (comment)

Do you think there would be any value to updating the docs with a note about doing this for an external/additional icon font library? I could do that.

Definitely value there, we're missing the top level comments that SUI core has. We should add those:

image

@kamdz
Copy link
Contributor

kamdz commented Feb 22, 2017

@levithomason should I add it in my pr?

@levithomason
Copy link
Member

@kamdz that would be great.

@kamdz
Copy link
Contributor

kamdz commented Feb 22, 2017

@levithomason I can't :/ I tried to add something like that:

/**
 * An icon is a glyph used to represent something else.
 * Icons serve a very similar function to text in a page. In Semantic icons receive a special tag <i> which allow for an abbreviated markup when sitting along-side text. 
 * Semantic includes a complete port of <a href="http://fontawesome.io/whats-new/">Font Awesome 4.7.0</a> designed by <a href="http://www.twitter.com/davegandy">Dave Gandy</a> for its standard icon set.

 * @see Image
 */

but it's not render well, it's look like a mess, and without hyperlinks. I give up this time, however without this, my pr is complete :)

@levithomason
Copy link
Member

Released in semantic-ui-react@0.67.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants