Skip to content
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

Helper#icon prepends fa- to given fa class string #106

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nnattawat
Copy link

I made the improvement on the helper method. Now it can handle multiple fa- classes.

For example, to add fa-2x class we need to use icon('flag', class='fa-2x') which make more sense to use it as the following.

icon('flag 2x')
# => <i class="fa fa-flag fa-2x"></i>

ex: icon('flag 2x') rather than icon('flag', class: 'fa-2x')
@@ -5,7 +5,8 @@ module ViewHelpers
def icon(icon, text = nil, html_options = {})
text, html_options = nil, text if text.is_a?(Hash)

content_class = "fa fa-#{icon}"
icons = icon.split(" ").map { |icon| "fa-#{icon}" }.join(" ")

Choose a reason for hiding this comment

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

Shadowing outer local variable - icon.

Copy link
Author

Choose a reason for hiding this comment

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

What do you mean?

@@ -4,8 +4,8 @@ module Rails
module ViewHelpers
def icon(icon, text = nil, html_options = {})
text, html_options = nil, text if text.is_a?(Hash)

content_class = "fa fa-#{icon}"
icon = icon.split(' ').map { |icon| "fa-#{icon}" }.join(' ')

Choose a reason for hiding this comment

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

Shadowing outer local variable - icon.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@nnattawat nnattawat force-pushed the handle-icon-array branch 2 times, most recently from c15208c to 18dec73 Compare November 12, 2015 02:23
@@ -5,7 +5,7 @@ module ViewHelpers
def icon(icon, text = nil, html_options = {})
text, html_options = nil, text if text.is_a?(Hash)

content_class = "fa fa-#{icon}"
content_class = "fa #{icon.split(' ').map { |i| "fa-#{i}" }.join(' ')}"

Choose a reason for hiding this comment

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

Line is too long. [81/80]

Copy link
Author

Choose a reason for hiding this comment

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

I think it's not that long haha :)

@nickpearson
Copy link

You should probably call to_s on icon before trying to split it. Before this change, a Symbol could be passed in, but now it requires a String, potentially causing existing usages to break.

@andreykul
Copy link

How about optional array instead?

icon([:flag, '2x', :fw], "Big Flag")
# => <i class="fa fa-flag fa-2x fa-fw"></i>

icon(:flag, 'Normal flag')
# => <i class="fa fa-flag"></i>

@@ -5,7 +5,8 @@ module ViewHelpers
def icon(icon, text = nil, html_options = {})
text, html_options = nil, text if text.is_a?(Hash)

content_class = "fa fa-#{icon}"
icons = icon.is_a?(Array) ? icon : icon.to_s.split(' ')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@nnattawat
Copy link
Author

@nickpearson and @andreykul good points guys. I have made it to support more formats.

icon(:flag)
# => <i class="fa fa-flag">
icon('flag')
# => <i class="fa fa-flag">
icon([:flag, :fw, '2x'])
# => <i class="fa fa-flag fa-fw fa-2x">
icon("flag fw 2x")
# => <i class="fa fa-flag fa-fw fa-2x">

…flag fa-2x'> now we can do:

- icon('flag 2x')
- icon([:flag, '2x'])
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants