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

Assets prefix #38

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

Conversation

Sinetheta
Copy link
Contributor

Although rails assets are generally in /assets they can be configured to be served from elsewhere. This breaks the demo.

The reason for 2 separate options is that I use this gem in non-rails projects, where my fonts and css aren't necessarily in the same place.

Since we allow users to specify the css and font asset directories separately, it makes sense that they should also be able to separate them for the purpose of serving up the demo.

I would like to write a guide for using this project without rails in the future (specifically middleman). Or we could split the project into 2 gems, with fontello_rails_converter being a thin wrapper on a more generic fontello gem.

So that we can access options from within it.
Although rails assets are _generally_ in /assets they can be
configured to be served from elsewhere. This breaks the demo.

The reason for 2 separate options is that I use this gem in non-rails
projects, where my fonts and css aren't necessarily in the same
place.

Since we allow users to specify the css and font asset directories
separately, it makes sense that they should also be able to
separate them for the purpose of serving up the demo.
@jhilden
Copy link
Contributor

jhilden commented Nov 16, 2015

thanks a lot for you enhancement. anything that makes the library less dependent on rails is very welcome. The plan of eventually extracting all the generic functionality into a new gem sounds like a very good approach for the future.

I'm very happy to merge your changes, however, I'm not so happy yet with the naming of the new config options and there is also some documentation missing for those options.

Unfortunately I don't really a have much better idea for naming either. Maybe instead of assets_prefix_css something like relative_path_to_compiled_css? What do you think?

So, please add some documentation (to the README) and give the naming a thought, then I will gladly merge your PR (and make a new release).

@Sinetheta
Copy link
Contributor Author

Sounds good. I'm a little swamped right now, but i'll make those changes when I get a chance.

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.

2 participants