search

seyhunak / twitter-bootstrap-rails

Twitter Bootstrap for Rails 6.0, Rails 5 - Rails 4.x Asset Pipeline
https://github.com/seyhunak/twitter-bootstrap-rails
4.49k stars 997 forks source link

Font Awesome now uses path variables #789

Closed joost closed 9 years ago

joost commented 10 years ago

Makes things work..

toadkicker commented 10 years ago

Can you explain a bit more?

joost commented 10 years ago

It now uses the less variables mentioned in the README. It got overwritten by @twiz718 in https://github.com/seyhunak/twitter-bootstrap-rails/commit/1b796b5d94780fb7d88e8cd0d2eb83bff2efe823.

toadkicker commented 10 years ago

There are several places that it is referenced; not just in path.less. To make this pull work we would need to update lib/generators/bootstrap/install/templates/bootstrap_and_overrides(.less,.css), as well as the static version in app/assets/stylesheets/fontawesome.css.erb

joost commented 10 years ago

The pull already works (I'm using it) .. the current master does not! bootstrap_and_overrides is what you use in your project (where you define the variables) and the fontawesome.css.erb is not used (in my case).

I agree that a better solution would be to be able to replace the fontawesome (less) files without them needed to be edited. Guess it should be possible by setting the fa-font-path variable.

But please merge so it actually works :)

toadkicker commented 10 years ago

1) When the next pull updates fontawesome, we're back to this problem again. 2) It breaks the documented way to set font paths because when the install generator runs it would be overridden anyway. 3) There isn't a test included that verifies the font path renders correctly, which would prevent this from happening again.

It's debatable if the current master is working for fontawesome or not, as many people are doing exactly what your changes describe in path.less, just that the path should be set outside the vendor provided files in order to stop the cycle of PR's for fixing paths.

joost commented 10 years ago
  1. true
  2. not true
  3. true

Check the generated bootstrap_and_overrides.css.less it sets the variables. This was the way you guys created it.. :S

toadkicker commented 10 years ago

Yes it sets it there in overrides, which is my point. @seyhunak, what do you think?

toadkicker commented 10 years ago

@joost take a peek at #802 and see if this fixes your issues.