Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat: add all font awesome icon names #211

Merged
merged 1 commit into from
Sep 10, 2018
Merged

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Sep 10, 2018

This PR adds all Font Awesome icons to the style Map, Icon typings, and Icon propTypes. This unblocks some of our consumers right away. It is also something we'll keep in the future, perhaps in the default theme so we're not adding throw away work here.

Copy link
Member

@miroslavstastny miroslavstastny left a comment

Choose a reason for hiding this comment

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

This is ~80kB (unzipped). In the feature we should think about moving it to a separate package?

@miroslavstastny miroslavstastny dismissed their stale review September 10, 2018 18:43

(Some of) the icons are broken

@miroslavstastny
Copy link
Member

image

@miroslavstastny
Copy link
Member

btw, the warning in console, if you use incorrect icon name is also cool:
image

@codecov
Copy link

codecov bot commented Sep 10, 2018

Codecov Report

Merging #211 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #211   +/-   ##
=======================================
  Coverage   90.17%   90.17%           
=======================================
  Files          55       55           
  Lines         916      916           
  Branches      142      142           
=======================================
  Hits          826      826           
  Misses         86       86           
  Partials        4        4
Impacted Files Coverage Δ
src/components/Avatar/Avatar.tsx 91.66% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ed93f6...82c7773. Read the comment docs.

@levithomason
Copy link
Member Author

This is ~80kB (unzipped). In the feature we should think about moving it to a separate package?

This size may have included typings and propTypes which shouldn't be part of the production bundle. However, I've removed the typings and propTypes for now. Themes will likely define their own icon names and icons. Until we resolve this, I don't think we should validate names.

Yes, this should be included in a separate package in the future, maybe the default theme.

(Some of) the icons are broken

Fixed.

btw, the warning in console, if you use incorrect icon name is also cool:

Fixed by relaxing the check to string.

Just FYI, customPropTypes.suggest() is capable of doing this (from SUIR):

image

@levithomason
Copy link
Member Author

This is passing on Screener and works locally. I'm not sure why icons weren't loading for you locally. However, I also have fix/merge-themes which fixes an issue with merging static styles and fonts in the theme.

I'm going to merge this PR adding icon names as is since it is passing and only adds the missing names to the existing pattern. I will then finish the merge themes fix before making a release.

@levithomason levithomason merged commit 7cde3dd into master Sep 10, 2018
@levithomason levithomason deleted the feat/font-awesome-icons branch September 10, 2018 21:37
@miroslavstastny
Copy link
Member

The reason is all the icons which do not work for me are from Font Awesome 5 Brands, not from Font Awesome 5 Free.
Icon component currently supports free and outline only, not brands:
https://github.com/stardust-ui/react/blob/7cde3ddf63bd406232b8f83d78c4f5b28fe77617/src/themes/teams/components/Icon/iconStyles.ts#L25

@levithomason
Copy link
Member Author

Heads up, I've fixed brand icons as well now.

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

Successfully merging this pull request may close these issues.

2 participants