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

Adding dependency for FontAwesome #26

Merged
merged 2 commits into from
Apr 6, 2015
Merged

Conversation

sctape
Copy link
Contributor

@sctape sctape commented Apr 2, 2015

Hey flatlogic,

I tried using your repo and was having trouble not realizing that the FontAwesome package was not being included. I noticed it was not listed as a dependency in bower and there was not an explicit mention of including the file in the readme. I eventually noticed that you were including the font-awesome.css file in your demo.

I made this pull request to hopefully make this more clear for future users of your repo.

Thanks,
Sam

@Siggen
Copy link

Siggen commented Apr 3, 2015

On the other hand, it is possible to use awesome-bootstrap-checkbox with Glyphicons instead of FontAwesome (see last paragraph of the readme), so I wonder if the lack of explicit dependency is intentional.

@sctape
Copy link
Contributor Author

sctape commented Apr 3, 2015

Sure. I get that. I was initially confused because there's no mention of FontAwesome in the "Use" section of the readme, and when I loaded the package the font was missing until I noticed it was included in the HTML of your demo. Just trying to help.

Thanks for sharing this cool plugin.

micrum added a commit that referenced this pull request Apr 6, 2015
Adding dependency for FontAwesome closes #27
@micrum micrum merged commit 81d6e29 into flatlogic:master Apr 6, 2015
@micrum
Copy link
Contributor

micrum commented Apr 6, 2015

Merged. Thx @sctape, @jimthedev, @Siggen

@jimthedev
Copy link

👍 thanks for the merge.

@sctape
Copy link
Contributor Author

sctape commented Apr 6, 2015

thanks!

@okendoken
Copy link
Member

@micrum, as @Siggen says bower dependency has not been specified intentionally in order to allow to use Glyphicons as a font. It also allows to use any specific FA or Glyphicons version. Reverting the merge

@sctape
Copy link
Contributor Author

sctape commented Apr 6, 2015

@okendoken that's totally cool. I would still advise updating the readme to better explain the "loose" dependency on FontAwesome. My minor frustration when I tried including this package was that the "Use" section of the readme does not mention including FontAwesome.css. Because of this, I was experiencing problems described in #9. After looking further at the demo file, I noticed the FontAwesome css file being included.

Or conversely, make the library use glyphicons by default instead of FontAwesome.

Just wanted to help the next person that uses this package avoid my oversight.

Thanks again for sharing and maintaining it.

@okendoken
Copy link
Member

@sctape you're fast ;)
just wanted to clarify that I agree that it would be great to show somewhere that font-awesome or glyphicons have to be specified in bower.json explicitly.
Creating #29 for that.

Thanks for collaboration

@micrum
Copy link
Contributor

micrum commented Apr 7, 2015

@okendoken that's right, but we use Font Awesome in main css, sass and less files as DEFAULT font-family.
So, specifying in dependencies Font Awesome is normal practice.

@jimthedev
Copy link

I have to agree with @micrum on this one. The current out-of-the-box experience is unclear because although there is an actual dependency in the code, it isn't made explicit in bower. New users get a broken initial experience unless they dig into the code.

In my mind, the most sane options are the following. The first and second options would be breaking changes and necessitate a major version bump, but seem ideal from a user perspective:

1. Remove the code that relies upon FA. Favor Glyphicons, the bootstrap default, out of the box

Bootstrap users likely aren't expecting font-awesome to be in the code unless there is an actual bower reference to it. Removing FA as the default seems sensible.

For a font-awesome specific version, a git hook could be added to automatically publish an alternate, font-awesome specific version to a newly created awesome-bootstrap-checkbox-fontawesome repo. This repo would just have two differences from the main repo. First, font-awesome declared in the bowerfile. Second, Glyphficons styles replaced by their font-awesome counterparts. Think of this as theming.

2. Remove all font-awesome code references and let userland override as needed

Users that really want font-awesome can always override the styles. We could even include an alternative css file that they could explicitly include if they want to use FA. Since this is a modification of bootstrap's functionality, I'd propose that this be treated as opt-in.

3. Keep the Font Awesome specific code in and formalize the dependency, since it already exists

The original proposal from @sctape.

Appreciate you all humoring the discussion.

@frazras
Copy link

frazras commented Jul 27, 2015

In case anyone just wants to get this workin quickly, add this to you code

<link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/font-awesome/4.3.0/css/font-awesome.min.css">

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.

None yet

6 participants