Closed (fixed)
Project:
Font Awesome Icons
Version:
7.x-2.1
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
31 Jul 2014 at 02:06 UTC
Updated:
24 Jan 2015 at 07:24 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
markhalliwellMoving to the correct project.
Comment #2
marty.true commentedI'm having the same issue. If we have the latest (4.1.0) library, how is the module not pulling from the lib?Ok after opening up the hood on the module itself, it appears that the icons are not in fact, being pulled from the library. The icons are in an array, hard-coded in the module. So now, this begs the question... why is the library dependent if the module is using hard-coded array variables?
Comment #3
marty.true commentedI created a patch that updates the module to the latest (4.1.0) icon set.
Comment #4
markhalliwellAfter downloading and looking at the library, it doesn't actually contain a list of icons. Just some css, fonts and precompiler mixins. My first thought is that you could parse the CSS file from the library, but it just uses a
.fa-prefix for all icons. It also uses that same prefix for it's modifiers (ie:.fa-lg, .fa-2x). How does one distinguish what is an icon and what is a modifier?I am guessing that this is why the icon list is a hard coded array. It wouldn't have to be if FA included, say, a JSON file that contained all the icons. This is how the services/modules like https://www.drupal.org/project/fontello and https://www.drupal.org/project/icomoon work (parsing the JSON file packaged with the "library"). Ironically, both these services provide FA icons ;) I personally don't really see the use case for downloading an entire library when only the typical site only uses a handful of icons to begin with, but that's a different topic lol
Comment #5
markhalliwellI wouldn't change this, it's out of scope of this issue and can affect existing sites with unintentional side-effects.
Use spaces, not tabs.
Needs newline at EOF.
Comment #6
marty.true commentedRemoved tabs and the fix for the correct default value
Comment #7
marty.true commentedComment #8
markhalliwellIt still needs to be indented, like it was before, just using 6 spaces:
After you do this, the patch should be much smaller.
Also you need to have a blank line at the end of the file, this is a git requirement.edit: I wasn't paying attention, you're actually adding a new line. Sorry.
Install https://dreditor.org and review the patch, you'll see what I'm talking about.
Comment #9
marty.true commentedIndents are done but I can't get the stupid "no newline" out. Sorry, my first patch... Any help?
Comment #10
marty.true commentedComment #11
marty.true commentedOne more try...
Comment #12
marty.true commentedComment #13
markhalliwellI really don't see how supporting FA 3 is going to work (or even why it's necessary... they don't support it). The icons array is a big mess and doesn't even change based on the library version. I think if anything the module should just keep up with the latest released version of FA. The project page can then state that for previous version support they can install an older version of the module (ie: for FA 3).
There is also some fundamental flaws with how this module has integrated Icon API support as well as some coding/documentation standards issues. I'll provide a patch shortly which should clear some of this up.
Comment #14
markhalliwellHere is a more comprehensive patch.
Re: icon array
I went ahead and took the liberty of sorting it alphabetically as well as removing duplicate entries based on @xstatic's patch in #9.
Comment #15
marty.true commentedGetting
Notice: Undefined index: tag in fontawesome_icon_bundle_configure()after applying the patch, and all the icons are gone (via the Icon API module).Comment #16
markhalliwellAh, sorry. I did the patch in my IDE (coding only), I didn't actually test it in browser. That being said, I got really involved with trying to clean up a lot of stuff as well as provide some Drush support so it would attempt to download the library automatically.
This module just needed a little bit of help. I would recommend that this be the 7.x-2.x version though. FA3 can remain coupled with 7.x-1.x, but given that their major version changed and their classes are very different, it is silly to support both in the same branch. Also, I changed the "download" url so it's actually pulling a specific version. The reason for this is: if they add a new icon (and we're always pulling their master version), then that wouldn't be in the icon list here.
Until there is a way to automatically parse the icons from FA, I don't see any other (feasible) way to not be version specific, otherwise things just end up breaking.
So here's the patch. Let me know what you think.
Comment #17
realityloop commentedUpdated for FA 4.2.0
Comment #19
inders commentedApplied patch for 7.x-1.x-dev. Now we have:-
- Drush support added
-- Usage: drush fadl or drush fa-download
- Support for latest v4.2.0 icons.
- We will consider V.2 for this also as changes are major.
Thank you all !
-Inder Singh
Comment #21
markhalliwellNot sure why you committed to 7.x-1.x at all, the changes are major, there's no getting around that. You should likely revert the commit on 7.x-1.x. Marking as "Fixed", it will close automatically after two weeks.
P.S. you should also look into the Credit & Committing fieldset at the bottom (giving credit) instead of manually constructing the commit message (this also lets you choose the author of the patch, so the commit credit appears on their profiles).
Comment #23
inders commentedHey Mark,
Yes I noticed that and reverted those changes from 1.x version.
So now we have:-
- v1.x - Font Awesome Library version v3.2.1
- v2.x - Font Awesome Library version v4.2.0
Both versions with drush support.
And Thank you for this credits link, I used that format but i think didn't work. Will give it another try.