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

Use font_path instead of asset_path #27

Merged
merged 1 commit into from May 14, 2013

Conversation

Projects
None yet
6 participants
@coderanger
Copy link
Contributor

commented May 14, 2013

This allows font-specific overrides.

Use the font_path instead of asset_path.
This allows font-specific overrides.
@bokmann

This comment has been minimized.

Copy link
Owner

commented May 14, 2013

This looks like a good change; I wasn't aware of the font_path helper method.

I'm not in a position to actually use this change on a project yet as a sanity check. @rmm5t can you verify and see if you can issue a release with the ownership permissions I gave you on rubygems.org?

@coderanger

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2013

For bonus points the reason I did this, if people are trying to use font-awesome-rails + Amazon CloudFront, you need something like this:

ActiveSupport.on_load(:action_view) do
  Rails.application.assets.context_class.instance_eval do
    old_font_path = instance_method(:font_path)
    define_method(:font_path) do |source|
      path = old_font_path.bind(self).(source)
      unless path.include?('?') || path.include?('#')
       path << '?'
       path << OpscodeWebui::Application.config.opscode.platform.origin
      end
      path
    end
  end
end

This is due to a poor interaction of Firefox's CORS policy on font files and CloudFront not understanding CORS headers.

@rmm5t

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2013

Sure thing. I'll take a look tomorrow morning. I thought font_path didn't work on older versions of rails, but it looks like the test suite passed which is a good start.

On Monday, May 13, 2013 at 8:11 PM, David Bock wrote:

This looks like a good change; I wasn't aware of the font_path helper method.
I'm not in a position to actually use this change on a project yet as a sanity check. @rmm5t (https://github.com/rmm5t) can you verify and see if you can issue a release with the ownership permissions I gave you on rubygems.org?


Reply to this email directly or view it on GitHub (#27 (comment)).

@coderanger

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2013

So another long-term note related to my hacking up font_path, right now the eot#iefix and svg paths will not be hash-stamped because sprockets (at least in my version) doesn't grok the query/fragment. Not sure if this is something you want to fix, but here is the revised version of the above override to fix it:

    define_method(:font_path) do |source|
      uri = URI(source)
      server_uri = URI(old_font_path.bind(self).(uri.path))
      server_uri.query = uri.query
      server_uri.fragment = uri.fragment
      server_uri.to_s
    end
@rmm5t

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2013

Okay, one small problem. font_path isn't available in Rails 3.1. The test suite wasn't covering this, but the gemspec allowed for it. In Rails 3.1, we get:

NoMethodError: undefined method `font_path' 

@bokmann, The gemspec currently mandates Rails >= 3.1. If we accept this PR, we'll have to bump that to Rails >= 3.2.

Personally, I'm perfectly okay with bumping the requirement to Rails 3.2. There's little reason for people to still be stuck on Rails 3.1 compared to 3.2 anyway, but I wanted to open the discussion before making a backwards incompatible change (though, Rails 3.1 folks can just continue to use the existing version of font-awesome-rails, and bundler should help manage this anyway).

I plan to noodle on this for a few hours awaiting discussion, but if no one objects, I'll move forward with the dependency bump to Rails 3.2.

@coderanger. Thanks for submitting this PR and the bonus CloudFront integration code. If you publish your CloudFront workaround as a gist or blog post, I'd be happy to link to it from the font-awesome-rails README. Just one thing. Please replace OpscodeWebui::Application.config with Rails.application.config.

Also, regarding eot#iefix and svg paths not getting hash stamped, I believe this was resolved in a new version of Sprockets, and as a result, I think Rails 4 handles this better.

@bokmann

This comment has been minimized.

Copy link
Owner

commented May 14, 2013

I like all of this. Lets clearly document in the readme that rails 3.1 or
older should use gem 'font-awesome-rails', '3.1.1.0' in their gemfile, and
lets make later version require rails 3.2 or later.

Thanks guys

-db

On Tue, May 14, 2013 at 9:57 AM, Ryan McGeary notifications@github.comwrote:

Okay, one small problem. font_path isn't available in Rails 3.1. The test
suite wasn't covering this, but the gemspec allowed for it. In Rails 3.1,
we get:

NoMethodError: undefined method `font_path'

@bokmann https://github.com/bokmann, The gemspec currently mandates
Rails >= 3.1. If we accept this PR, we'll have to bump that to Rails >=
3.2.

Personally, I'm perfectly okay with bumping the requirement to Rails 3.2.
There's little reason for people to still be stuck on Rails 3.1 compared to
3.2 anyway, but I wanted to open the discussion before making a backwards
incompatible change (though, Rails 3.1 folks can just continue to use the
existing version of font-awesome-rails, and bundler should help manage this
anyway).

I plan to noodle on this for a few hours awaiting discussion, but if no
one objects, I'll move forward with the dependency bump to Rails 3.2.

@coderanger https://github.com/coderanger. Thanks for submitting this
PR and the bonus code for CloudFront integration. If you publish your
CloudFront workaround as a gist, I'd be happy to link to it from the
font-awesome-rails README. Just one thing. Please replace
OpscodeWebui::Application.config with Rails.application.config.

Also, regarding eot#iefix and svg paths not getting hash stamped, I
believe this was resolved in a new version of Sprockets, and as a result, I
think Rails 4 handles this better.


Reply to this email directly or view it on GitHubhttps://github.com//pull/27#issuecomment-17877435
.

rmm5t added a commit that referenced this pull request May 14, 2013

Merge pull request #27 from coderanger/master
Use font_path instead of asset_path

@rmm5t rmm5t merged commit e6063d6 into bokmann:master May 14, 2013

1 check passed

default The Travis CI build passed
Details

rmm5t added a commit that referenced this pull request May 14, 2013

Upped railties dependency from 3.1 to 3.2
Related to using font_path instead of asset_path as described in #27.
@rmm5t

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2013

@coderanger, I just released font-awesome-rails v3.1.1.1. Thanks again for the contribution!

@balexand

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2013

👍 on using font_path.

But monkey patching font_path or bypassing the CDN is not a good way of dealing with the CORS issue.

A much better/easier solution would be to serve font assets with the Access-Control-Allow-Origin header set. This is compatible with CloudFront (I know because I'm using it in production). If you're using CloudFront as a reverse proxy, then it will pass any CORS headers from the origin server. How to configure the header depends on what you're using on your server, but Nginx, S3, and Rack::Static all provide ways to configure this header.

It would be nice to see documentation for this in the README. Personally, I was taken by surprise when I first learned that Firefox and IE 9+ require the CORS headers in order to load font assets from a different domain.

@coderanger

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2013

@balexand I use the same bucket to drive multiple domains and Cloudfront doesn't support Vary: Origin on caching :-)

@rmm5t

This comment has been minimized.

Copy link
Collaborator

commented Jun 3, 2013

👍 to getting this information incorporated into the README. I'm not an expert on the topic, but would love to see a PR, gist, blog post, or something that we could reference.

@xdite

This comment has been minimized.

@masonforest

This comment has been minimized.

Copy link

commented Sep 23, 2013

Here's a good one for hosting on heroku: http://singlebrook.com/blog/cloudfront-cdn-with-rails-on-heroku

@rmm5t rmm5t referenced this pull request Apr 16, 2014

Closed

Updating Asset Host #87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.