Skip to content
This repository
Browse code

Merge pull request #4803 from lucascaton/master

Remove slash from favicon_link_tag method attribute
  • Loading branch information...
commit bb842e8d2111e50b21a14b8bd6d89371a4b9cd68 2 parents 40c287c + a5976cc
Aaron Patterson authored
2  actionpack/CHANGELOG.md
Source Rendered
@@ -26,6 +26,8 @@
26 26
     This is a behavior change, previously the hidden tag had a value of the disabled checkbox.
27 27
     *Tadas Tamosauskas*
28 28
 
  29
+*   `favicon_link_tag` helper will now use the favicon in app/assets by default. *Lucas Caton*
  30
+
29 31
 ## Rails 3.2.0 (January 20, 2012) ##
30 32
 
31 33
 *   Add `config.action_dispatch.default_charset` to configure default charset for ActionDispatch::Response. *Carlos Antonio da Silva*
4  actionpack/lib/action_view/helpers/asset_tag_helper.rb
@@ -234,7 +234,7 @@ def auto_discovery_link_tag(type = :rss, url_options = {}, tag_options = {})
234 234
       #
235 235
       # generates
236 236
       #
237  
-      #   <link href="/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />
  237
+      #   <link href="/assets/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />
238 238
       #
239 239
       # You may specify a different file in the first argument:
240 240
       #
@@ -252,7 +252,7 @@ def auto_discovery_link_tag(type = :rss, url_options = {}, tag_options = {})
252 252
       #
253 253
       #   <%= favicon_link_tag 'mb-icon.png', :rel => 'apple-touch-icon', :type => 'image/png' %>
254 254
       #
255  
-      def favicon_link_tag(source='/favicon.ico', options={})
  255
+      def favicon_link_tag(source='favicon.ico', options={})
256 256
         tag('link', {
257 257
           :rel  => 'shortcut icon',
258 258
           :type => 'image/vnd.microsoft.icon',
2  actionpack/test/template/asset_tag_helper_test.rb
@@ -168,7 +168,7 @@ def teardown
168 168
   }
169 169
 
170 170
   FaviconLinkToTag = {
171  
-    %(favicon_link_tag) => %(<link href="/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />),
  171
+    %(favicon_link_tag) => %(<link href="/images/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />),
172 172
     %(favicon_link_tag 'favicon.ico') => %(<link href="/images/favicon.ico" rel="shortcut icon" type="image/vnd.microsoft.icon" />),
173 173
     %(favicon_link_tag 'favicon.ico', :rel => 'foo') => %(<link href="/images/favicon.ico" rel="foo" type="image/vnd.microsoft.icon" />),
174 174
     %(favicon_link_tag 'favicon.ico', :rel => 'foo', :type => 'bar') => %(<link href="/images/favicon.ico" rel="foo" type="bar" />),

8 notes on commit bb842e8

Santiago Pastorino

why /images here and /assets in the docs?

Santiago Pastorino
Owner

@tenderlove @lucascaton I've just reverted this commit. Look for an explanation here 6871bd9

Lucas Caton

Ok @spastorino, no problem :-)

But this way, we'll have to put the favicon in the public directory instead of putting the assets/images directory :-(

Xavier Noria
Owner

No, you can pass a path to the helper.

Lucas Caton

Ok, but the default (no parameter) should find the favicon in app/assets/images ;)

Xavier Noria
Owner

Yeah I agree with you, the new default was backwards incompatible, but aligned with the asset pipeline.

Santiago Pastorino
Owner

We can come up with a better solution but if going to be backwards incompat at least we should clearly document it.
@jeremy thoughts?

Santiago Pastorino
Owner

I've re-reverted this 3893979 given that scaffold doesn't generate favicon_link_tag it's not a bad change :). Thanks guys!

Please sign in to comment.
Something went wrong with that request. Please try again.