-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
FontAwesome 5 with fallback #787
Conversation
+ GlyphMap for FontAwesome 5.1.0 Pro + Font files for FontAwesome 5.1.0 Free + Implementation enabling FontAwesome5 icons Handles the copying and renaming of FontAwesome5 Pro font files Removed debug code
Now a default import from a separate file, instead of creating it using FA5iconSet(bool).
Fixes to pass tests
+ FontAwesome 5.1.1 + Temporary folder and npm pack when building + Separate glyphmaps
f4eed57
to
642e989
Compare
642e989
to
50b793b
Compare
if (brand) return BrandsSet; | ||
if (light) return LightSet; | ||
if (solid) return SolidSet; | ||
if (brand) return FA5Style.brand; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought we were able to remove the brand
prop with this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is related to my last comment 👇🏻
@@ -0,0 +1,3491 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a very verbose data structure, could we invert it by doing { "brands": ["500px"...] ... }
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 🙂
glyphMap, | ||
metadata = {}, | ||
proVersion = false, | ||
fallback = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this fallback
argument if it's always true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was meant to give more control in case some people want to disable the fallback (for whatever reason). It adds no complexity to normal users but adds some options for those who wish to do this. If it's disabled then the brand
prop is needed (with the fallback it can be omitted). 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly don't see the need for the brand
prop, it's not really a variant, just a different font.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey, I'll just remove the brand
prop and the fallback option!
@hampustagerud This is blocking release, do you have time to address my comments? 😃 |
directory/src/App.js
Outdated
let familyName = family; | ||
|
||
if (family === 'FontAwesome5') { | ||
if (FontAwesome5Meta[name].indexOf('solid') === -1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess this test is now broken
const generatedJSON = {}; | ||
|
||
const path = `${argv.path}/svgs/`; | ||
fs.readdirSync(path).forEach(file => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we refactor this to a filter/map
instead?
@@ -8,6 +8,7 @@ | |||
"adn": 61808, | |||
"adversal": 62314, | |||
"affiliatetheme": 62315, | |||
"air-freshener": 62928, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we suddenly get new free icons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! I've been following this and felt like chiming in. :) Seems this icon and all the others added here came from the 5.2 release that came out this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, when I worked on the metadata generator it automatically updated the fonts so I just put them in there as well 🙂
I tried to implement the metadata generator using filter/map but ended up using filter/forEach in combination since I'm not really sure how to create the object otherwise 🙂 |
Thanks so much for your hard work and swift resolution of comments @hampustagerud 👍 |
Added fallback which enables IconExplorer and directory to show all icons. Fallback is implemented using metadata automatically generated from FontAwesome 5 npm packages.