Icon helper #14

buren · 2014-09-11T23:47:11Z

Created IconHelper
Removed FontAwesome helper
Removed Glyphicon helper

coveralls · 2014-09-11T23:49:33Z

Coverage remained the same when pulling 2db9e45 on buren:icon-helper into d152f2e on Fullscreen:master.

buren · 2014-09-11T23:51:03Z

@claudiofullscreen this is supports everything that used to work and unified glyphicon and font-awesome under one helper.

The only problem is that in BaseHelper#error_icon_for there is no way for the caller to pass library: :font_awesome.

Don't really know how to fix that without having some config var for that.

claudiofullscreen · 2014-09-11T23:52:09Z

lib/bh/helpers/icon_helper.rb

😮 I didn't know font awesome had more options!

https://fortawesome.github.io/Font-Awesome/examples/#larger

Mmmh… I need to take a look at those options! It's a good idea to add them, but I still wouldn't want the code to be too specific to Font Awesome.

For instance, if someone writes

<%= icon :heart, library: :glyphicons, size: 3 %>

I wouldn't mind displaying <span class="glyphicon glyphicon-zoom-in glyphicon-3x"></span> even if that class does not have a meaning for glyphicon.

At least there would be more consistency in the helper, and less library-specific options.

I'll have to look up those options!

claudiofullscreen · 2014-09-11T23:56:53Z

@buren As I said in the comment to your other PR, there is no need to customize the error icon.

That helper is there to show a red "X" at the right of an invalid field… it's a fixed universally recognized symbol to display that something is wrong, so I don't think that's something that needs to be customizable.

If it was, then someone might argue that the symbol itself should be customizable: instead of showing an "X" someone would want to show a telephone… I don't think it's worth going there.

Once again, whenever you have Bootstrap CSS, you do have that "X" symbol, so we can just use that.

claudiofullscreen · 2014-09-11T23:59:18Z

lib/bh/helpers/icon_helper.rb

I think this line is superfluous…

which lines? 22-25?

No, just line 30…

Not sure what you mean, how would you want it?

buren · 2014-09-12T00:00:56Z

OK.

I have nothing to add beyond what is in this pull request. Is there anything you think is missing?
(I didn't bump the version and updated the changelog guessed that you perhaps wanted to do it?)

claudiofullscreen · 2014-09-12T00:01:03Z

lib/bh/helpers/icon_helper.rb

👍 I thought the same… actually I would just go with options[:library].to_s so you can also catch font_awesome… or even better, since ActiveSupport is already required, you can call .underscore, so all the variants are transformed into font_awesome

claudiofullscreen · 2014-09-12T00:04:47Z

I think this PR looks really solid now (I have added my last comments to the code).

I have to go through and learn a little more about Font Awesome (to make sure it does not clash with Glyphicons), but in general I would say 👍

coveralls · 2014-09-12T00:14:17Z

Coverage remained the same when pulling 5b842a0 on buren:icon-helper into d152f2e on Fullscreen:master.

coveralls · 2014-09-12T00:17:10Z

Coverage remained the same when pulling dc68503 on buren:icon-helper into d152f2e on Fullscreen:master.

claudiofullscreen · 2014-09-12T00:18:14Z

lib/bh/helpers/icon_helper.rb

If I read your case statement correctly, it says

case options.delete(:library) when :font_awesome, 'font-awesome' then :fa else options[:library] :glyphicon end

So I have a feeling that when the else statement is reached, then :glyphiconis always returned, rather than options[:library].
Maybe it's just me reading the code on GitHub…

No thats my bad! Its getting a little late for my eyes I guess...

coveralls · 2014-09-12T00:19:58Z

Coverage remained the same when pulling f5f9319 on buren:icon-helper into d152f2e on Fullscreen:master.

buren · 2014-09-12T00:21:34Z

Added the .underscore as you suggested and I think that I've addressed all your comments. I'm ready if travis is :)

coveralls · 2014-09-12T00:21:38Z

Coverage remained the same when pulling 64ff0b2 on buren:icon-helper into d152f2e on Fullscreen:master.

buren · 2014-09-12T00:31:22Z

wait found one little problem, fixing it

claudiofullscreen · 2014-09-12T00:33:06Z

Ah, don't worry! I'm not in a hurry. Also, if you plan to use this new feature in a project of yours, go on and link the Gemfile to your own branch, so you can try it a little more on a real application, and give me your impressions!

buren · 2014-09-12T00:36:09Z

Absolutely!

Im thinking of just aliasing the glyphicon method to icon if thats alright?

coveralls · 2014-09-12T00:41:27Z

Coverage remained the same when pulling a70a8fb on buren:icon-helper into d152f2e on Fullscreen:master.

buren · 2014-09-12T00:41:31Z

I went ahead and used alias_method :glyphicon, :icon which ensures backward compatibility. There is also a test for it in spec/helpers/glyphicon_helper_spec.rb

claudiofullscreen · 2014-09-12T00:42:21Z

Mmmh… no, I'd rather have glyphicon explicitly call icon with the
correct option, so it's independent of the implementation!

Anyway… don't worry too much about these details! I'll review it all and
give it a 👍 then! You already helped immensely :)

On Thu, Sep 11, 2014 at 5:36 PM, Jacob Burenstam notifications@github.com
wrote:

Absolutely!

Im thinking of just aliasing the glyphicon method to icon if thats
alright?

—
Reply to this email directly or view it on GitHub
#14 (comment).

Claudio Baccigalupo | Software Writer | Fullscreen, Inc.
http://Fullscreen.net | +1.310.202.3333

* Works with both Glyphicon and Font Awesome * Removed FontAwesome and Glyphicon Helper * 100% backward compatible

coveralls · 2014-09-12T07:49:11Z

Coverage remained the same when pulling fc1eb25 on buren:icon-helper into d152f2e on Fullscreen:master.

buren · 2014-09-12T16:17:47Z

@claudiofullscreen https://github.com/buren/bh/tree/additional-components adds support for progress-bars, labels, badges and wells.

claudiofullscreen · 2014-09-12T16:21:22Z

@buren 😮 I don't know how to thank you!! 👍 I'm going to go and read through those commits as soon as I can!

buren · 2014-09-12T18:49:03Z

Closing this favouring: #16

buren added 2 commits September 12, 2014 00:18

Added Font Awesome helper.

7fcab15

Added support for Font Awesome option classes

9494617

claudiofullscreen reviewed Sep 11, 2014
View reviewed changes

buren mentioned this pull request Sep 11, 2014

Added Font Awesome helper. #13

Closed

claudiofullscreen reviewed Sep 11, 2014
View reviewed changes

claudiofullscreen reviewed Sep 12, 2014
View reviewed changes

buren force-pushed the icon-helper branch from 2db9e45 to 5b842a0 Compare September 12, 2014 00:12

buren force-pushed the icon-helper branch from 5b842a0 to dc68503 Compare September 12, 2014 00:15

claudiofullscreen reviewed Sep 12, 2014
View reviewed changes

buren force-pushed the icon-helper branch from dc68503 to f5f9319 Compare September 12, 2014 00:18

buren force-pushed the icon-helper branch from f5f9319 to 64ff0b2 Compare September 12, 2014 00:20

buren force-pushed the icon-helper branch from 64ff0b2 to a70a8fb Compare September 12, 2014 00:39

Added icon helper method.

fc1eb25

* Works with both Glyphicon and Font Awesome * Removed FontAwesome and Glyphicon Helper * 100% backward compatible

buren force-pushed the icon-helper branch from a70a8fb to fc1eb25 Compare September 12, 2014 07:47

claudiofullscreen mentioned this pull request Sep 12, 2014

Icon helper #16

Merged

buren closed this Sep 12, 2014

Icon helper #14

Icon helper #14

Uh oh!

Conversation

buren commented Sep 11, 2014

Uh oh!

coveralls commented Sep 11, 2014

Uh oh!

buren commented Sep 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudiofullscreen commented Sep 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buren commented Sep 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

claudiofullscreen commented Sep 12, 2014

Uh oh!

coveralls commented Sep 12, 2014

Uh oh!

coveralls commented Sep 12, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Sep 12, 2014

Uh oh!

buren commented Sep 12, 2014

Uh oh!

coveralls commented Sep 12, 2014

Uh oh!

buren commented Sep 12, 2014

Uh oh!

claudiofullscreen commented Sep 12, 2014

Uh oh!

buren commented Sep 12, 2014

Uh oh!

coveralls commented Sep 12, 2014

Uh oh!

buren commented Sep 12, 2014

Uh oh!

claudiofullscreen commented Sep 12, 2014

Uh oh!

coveralls commented Sep 12, 2014

Uh oh!

buren commented Sep 12, 2014

Uh oh!

claudiofullscreen commented Sep 12, 2014

Uh oh!

buren commented Sep 12, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants