I was digging in some old Basecamp code today while working on a new feature and I ran into this bit of view code:
<div class="rss">
<%= rss_link :icon %>
<p><%= rss_link "Project RSS feed" %></p>
</div>
This code renders an RSS icon wrapped in a link and a text link beside it:
I wanted to add a class to the linked RSS icon. If the template used the standard Rails link_to helper to render the linked image I would know how to provide the class with an options hash. But rss_link isn’t a standard helper, so I had to dig up the definition to see what’s possible. It wasn’t very pretty:
def rss_link(body, options={})
if body == :icon
body = image_tag("feed-icon-14x14.png", :size => "14x14", :alt => "RSS Feed",
:border => "0", :align => "absmiddle")
end
link_to(body, { :controller => "project", :action => "rss_confirm",
:return_to => request.request_uri }, options)
end
It turned out we were using a single helper to render either a text link or a linked icon for the RSS feed depending on the ‘body’ param. It works fine, but I would have never been able to guess how it works without looking inside the method definition. Now I can see that it is possible to pass an options hash, but I had to look to find that out.
It would have been better in this case to use the standard link_to in the template and delegate the truly unique stuff to helper methods. The unique bits that I don’t want to repeat are the long url_for hash and the image tag params. Here’s a refactoring that splits the helper into two methods to return only those special parts:
def rss_path
{ :controller => "project", :action => "rss_confirm",
:return_to => request.request_uri }
end
def rss_icon
image_tag("feed-icon-14x14.png", :alt => "RSS Feed", :class => "rss_icon")
end
These helpers allow me to stick with link_to, which is a helper that I and my team already understand:
<div class="rss">
<%= link_to rss_icon, rss_path, :class => 'image' %>
<p><%= link_to "Project RSS feed", rss_path %></p>
</div>
It’s much easier to tell at a glance what this code is doing and it’s easier to make changes because the link_to helper is standard.
The next time we want to change this bit of view code, we won’t have to dig into the helper to check if it takes an options hash. It’s clear from the call to link_to that we’re simply providing method calls as params. I hope this tip helps to make your view code a bit more readable and more predictable to maintain.
Chris Vincent
on 15 May 09Sometimes the tiniest of refactorings can make all the difference in making code easier to understand and maintain. I’ve run into this particular issue with view helpers before; this is a refreshing solution. Thanks!
Melvin Ram
on 15 May 09One of the best technical posts on here in a while. It’s a great example of good tastes & judgement that comes with time. Thanks!
Vincent Robert
on 15 May 09Awesome refactoring! Now to go a little deeper, you could just display your RSS icon using a padding and a background image on the text link.
Rimantas
on 15 May 09+1 to Vincent’s suggestions. The link text is self-explanatory, so this icon belongs in CSS. One less HTTP request. And then come CSS sprites…
Daniel Lopes
on 15 May 09Thanks Ryan, please continue posting about source code. Bye.
Michael Brooks
on 15 May 09Ryan, nice use of a custom helpers to actually help the built-in rails functions.
Chris White
on 15 May 09I love your products, but I can’t find any other way to contacy 37 signals. I have a great idea I would like to share to make TADA lists even simpler. (Better Design) I will give it to you free, I’m not a developer, but I want to help. [email protected]
Chris Vincent
on 15 May 09Another +1 to Vincent Robert’s suggestion; the RSS icon is really in the presentation layer and so should be a CSS background image. Plus, I’m a big fan of the name “Vincent”.
Daniel Harrington
on 15 May 09great post. i’d really love to see (read) more of those! and another +1 to vincent by the way.
RS
on 15 May 09I wouldn’t put the RSS icon in the CSS as some of you suggest. Moving the icon to the CSS adds an unnecessary layer of indirection to the code. In other words, when I look at the template, I’ll be seeing code for a link but not code for an image. Then when I look at the screen, an image appears there. The CSS effectively hides the image rendering from the person who is viewing the code, whether that’s myself in 6 weeks or another team member. Rendering the image directly from the markup instead is clearer for purposes of maintenance and code readability.
Chris Vincent
on 15 May 09You make a good point, but it is always the nature of CSS that presentation aspects are going to be defined separately from the structure of the page. For me, it just helps to be consistent. If an image is part of the design and is going to be the same in different pages of the site or reused multiple times on the same page, then it goes into the CSS. I only use an img element where the image is an explicit part of the content of the page, such as a photo in an article or an infographic.
Of course, the practical difference here is ultimately a matter of style; there are a few instances where the lines blur, but you could call me a purist with regard to presentation vs structure. :)
MattH
on 16 May 09“Now I can see that it is possible to pass an options hash, but I had to look to find that out.”
No “intellisense” with Rails apps?
Adam
on 16 May 09I always love these code posts – this is the kind of stuff that keeps bringing me back here.
I do agree though, an RSS icon belongs in CSS as the smart folks above had mentioned. The argument of using an image for the sake of clarity for programmers doesn’t seem nearly as important as clarity for the end user.
Instead of an image and link inside a paragraph inside a div (yikes), you can simply do this:
<a rel=”rssLink” href=”#”>Project RSS Feed</a>
You don’t support IE6 anyway, so you can hit it with CSS with a[rel=”rssLink”]. In my opinion this gives plenty of clarity to the programmers while providing clean output to the user as well.
Yes, it sounds picky, but this kind of markup adds up very quickly. A simple DOM means faster JS, cleaner and lighter CSS, SEO-friendliness, and less cross-browser variation.
John Topley
on 17 May 09A good reason not to put the feed icon in the CSS is that doing so prevents it from having alt text or a title.
Mathew Patterson
on 17 May 09@John
I agree that you could go either way with the image, but in the case of the image being in the CSS, you’d still have a title on the link, so there is no accessibility issue there.
Andreas Lanjerud
on 18 May 09In this case, the image is just a visual extra and doesn’t really add something to the function. So putting it into the CSS (neat inside a sprite background) would be all gain. Classes are fine to use too:
<a class=”rss_feed” href=”...”>Project RSS Feed</a>
Better HTML, better HTTP-requests, easier backend
Chris Lloyd
on 19 May 09Should be “my team and I”.
Anonymous Coward
on 19 May 09No it shouldn’t Chris.
Rimantas
on 20 May 09“Rendering the image directly from the markup instead is clearer for purposes of maintenance and code readability.”
Sadly, that’s the idea of CSS – to separate markup from presentation.
@John – with the link text “Project RSS feed” any alt text is redundant and hurts more than helps.
RS
on 21 May 09I smell kool-aid. The RSS icon isn’t a style. It’s not a rounded corner or a gradient background. It’s a design element with its own utility. It has a meaning, it does something when you click on it, and it plays a role in the layout of the page. If it was meaningless, I wouldn’t have included it in the design. There’s no good reason by default to hide it from the markup.
This discussion is closed.