Skip to content

Conversation

@claudiofullscreen
Copy link
Contributor

Hello @buren ! This PR stems from #14 – let me tell you what I've changed:

  1. I have squashed your commits into one
  2. I have temporarily removed the additional options of Font Awesome icons (size, fixed width). I don't mean to remove them forever, I just need more time to think about the option names. Meanwhile, already with this PR, you can still have different sizes by using the :class option, such as <%= icon :heart, library: :font_awesome, class: 'fa-2x' %>
  3. I have added a font_awesome_css helper to CDN, so it's even easier to use Font Awesome icons.
  4. I have cleaned up the documentation and the specs a little bit
  5. I have left the Glyphicon helper for now… for backwards-compatibility and I don't mind having two methods in this case.

If it all looks good to you, I'll merge and add the appropriate documentation to http://fullscreen.github.io/bh

Thanks again for the idea and the coding!!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling c9ffc58 on icon-helper into d152f2e on master.

buren and others added 2 commits September 12, 2014 10:59
Adds a level of abstraction to the existing `glyphicon` helper, allowing
users to select a different vector icons library. For instance, it is
now possible to display Font Awesome icons by passing the library option:

    <%= icon :heart, library: :font_awesome %>

Note that, since Font Awesome CSS is not loaded by default with Bootstrap
CSS, it's up to the user to make sure the CSS is loaded in the same app.
Since the `icon` helper accepts `library: :font_awesome`, and the
Font Awesome CSS is not loaded by default by Bootstrap CSS, it is
helpful to have an additional CDN helper, so people who want to
include Font Awesome icons can simply write:

```rhtml
<%= stylesheet_link_tag font_awesome_css %>
<%= icon :heart, library: :font_awesome %>
```
@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 22a6813 on icon-helper into d152f2e on master.

@buren-trialbee
Copy link

+1

@buren buren mentioned this pull request Sep 12, 2014
claudiofullscreen pushed a commit that referenced this pull request Sep 12, 2014
@claudiofullscreen claudiofullscreen merged commit 3485dd2 into master Sep 12, 2014
@claudiofullscreen claudiofullscreen deleted the icon-helper branch September 12, 2014 18:58
@buren
Copy link
Contributor

buren commented Sep 12, 2014

@cladiofullscreen how would you like me to integrate this with #15, straight merge?

@claudiofullscreen
Copy link
Contributor Author

Since this is on master, the way to do it would be to rebase #15 on top of master.

However, I have a feeling that #15 might be split into separate commits/PRs, so I wouldn't worry about that now 😊

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.

5 participants