jessesquires / BButton

Twitter Bootstrap buttons for iOS
http://cocoadocs.org/docsets/BButton/
Other
712 stars 82 forks source link

Updated TTF file with the latest 4.0.3 version of the FontAwesome direct... #22

Closed lucasiturbide closed 10 years ago

lucasiturbide commented 10 years ago

...ly from the Website.

Updated the NSString category with the latest icon. A refactor was needed to update easyly the icon values and a better way to convert to FA string from an Enum value in HEX.

Deprecated unnecessary methods. Keeps the backwards compatibility of the Icon names.

The Enum has the same name convention than the CSS file on FA website, but keeps the older names for backward compatibility.

Fixed the Random icon generator in Demo App accordingly to the new enum based icons.

jessesquires commented 10 years ago

thanks again @lucasiturbide !

using hex was a good idea - definitely much simpler. however, specifying each enum value makes this difficult to maintain. (but it is still much better than before).

would this work?

Don't specify enum values:

typedef NS_ENUM(NSUInteger, FAIcon){
    FAIconGlass,
    FAIconMusic,
    FAIconSearch,
    ....
}

Then simply use the hex format specifier:

+ (NSString *)fa_stringForFontAwesomeIcon:(FAIcon)icon {
    return [NSString stringWithFormat:@"%x", icon];
}

Other issues:

  1. I'm not interested in backwards compatibility and deprecating methods. Anything not used should simply be removed. This project abides by semantic versioning. These changes will result in a new release, 4.0.0.
  2. We should follow the exact naming conventions from the web.
  3. Travis is still broken unfortunately, but I'm pretty sure these changes break existing unit-tests. You should run the unit-tests and update accordingly.
  4. Other comments are in-line
lucasiturbide commented 10 years ago

For the enum values, the answer is no. That won't work, because in the enums if you don't assign a value, it will start from 0 to infinity, sequentially. That's why I assign the hex value for each value in the enum. I took the hex values from the CSS file, some several minutes with a text editor that has a regex find&replace utility. For the backward compatibility, if you want I can remove everything that is not necessary anymore instead of deprecating the methods, but imagine that someone wants to update the whole library to use the latest icons from FontAwesome and they have spread in all the code, all the methods with the old enum values. That will be a pain in the ass to update the library and use the latest values. That's why I took the time to match the old enum values with the HEX codes. Regarding the naming convention, I couldn't use the exact names from the CSS because objective-c will not allow you to use a dash in the name of the values (like fa-foursquare). Again the magic regex did the work for me. Let me know you thoughts about this.

lucasiturbide commented 10 years ago

Hi Jesse, I think the build is failing because the test should run on iOS 7.0+ and not on 6.0 like travis is trying to run the tests. I don't know how to change it yet. Let me know if you can figure out the problem.

jessesquires commented 10 years ago

hey @lucasiturbide - tests are running for iOS 7.

seems to be a known issue with travis. i tried tweaking the script more but it is still failing. (though everything works fine for me locally).

issue opened with travis here: https://github.com/travis-ci/travis-ci/issues/2179

jessesquires commented 10 years ago

@lucasiturbide - issue fixed! details available on the thread in the issue noted above.

please update your PR with the latest from develop! :smiley:

lucasiturbide commented 10 years ago

@jessesquires I did rebase and pushed again, I believe it's ready for merging.

jessesquires commented 10 years ago

thanks for the update @lucasiturbide ! i have a few more changes and questions.

specifying the hex value for each icon is too much work, we can do this instead:

typedef NS_ENUM(unsigned short, FAIcon){
    FAIconGlass = 0xf000,
    FAIconMusic,
    FAIconSearch,
    FAIconEnvelope,
    FAIconHeart,
    FAIconStar,
    FAIconStarEmpty,
    FAIconUser,
    FAIconFilm,
    FAIconThLarge,
    FAIconTh,
    FAIconThList,
    FAIconOk,
    FAIconRemove,
    FAIconZoomIn,
    FAIconZoomOut = 0xf010,
    FAIconOff,
    FAIconSignal,
    .......

Unless there's a reason we shouldn't?

Normally, only having the first assignment would be ok (FAIconGlass = 0xf000) however, it appears that these are not consecutive values, thus the need to reset at FAIconZoomOut (which is 0xf010, but would be 0xf00f if these were consecutive).

So we will have to explicitly set the value every few icons. (There doesn't seem to be a pattern.) I wonder why FontAwesome does this. :cry:

lucasiturbide commented 10 years ago

@jessesquires I've removed the deprecated methods and fixed the random generated icons. Regarding the values in the enum, I don't think that using the method of setting only the first value will be a good approach, since the values are not consecutive, and there are some duplicated values, which have the name convention that you were using. Maybe we can remove those values if you think that they are not needed anymore.

jessesquires commented 10 years ago

@lucasiturbide - nice! :+1: Sounds good about the enums, that's probably best. (I wish they were consecutive!)

  1. Duplicates - yes let's remove them
  2. Naming - let's follow exactly what FontAwesome does (i.e., FAPencilSquare, FAPencilSquareO, etc.)

Thanks for all of your hard work! :smile:

lucasiturbide commented 10 years ago

Hi @jessesquires, I've removed duplicated old entries and fixed the tests and demo with new values!

jessesquires commented 10 years ago

hey @lucasiturbide - thanks so much! looks great!

jessesquires commented 10 years ago

sorry for the delay, been super busy!