I’m working on some improvements to Basecamp, specifically the screens where you manage which people have access to a project. There’s an area on our new template with checkboxes beside peoples’ names so you can check which people should be added to your project. I want to apply a class name to the label tag around the checkbox for each person. So I pulled up the template and searched for “label” to find where I might add the class name. There were no matches. So I dug deeper, and saw that the HTML for the label, checkbox, and person name is being generated by a Rails helper method.
This is the template code that calls the helper method.
<% people_without_access_from(company).each do |person| %>
<%= add_person_to_project_check_box(person, company) %>
<% end %>
Next I pulled up the helper method to see if it really is responsible for producing a chunk of HTML with the label and checkbox.
def add_person_to_project_check_box(person, company)
content_tag(:label,
check_box_tag("people_ids[]", person.id, false, { :class => "company_#{company.id}_person" }) +
" " + person.name
) + tag(:br)
end
Yup, it’s generating the HTML. Right away this smells bad to me. The helper is first generating a label tag. Inside that label, there is a checkbox followed by a space character and the person’s name. Finally a break is appended after the label. This smells bad for two reasons. First, it’s just not so nice for helpers to cook out HTML when they don’t have a good reason to. Second, it’s harder to locate and change HTML when it’s hidden inside a helper.
Returning to my original goal, I wanted to add a class name to the label around this checkbox. If I add the class name to the existing helper, it’s going to get even more messy and complicated because I have to give content_tag an attribute with the class name I want. It would look like this:
def add_person_to_project_check_box(person, company)
content_tag(:label,
(check_box_tag("people_ids[]", person.id, false, { :class => "company_#{company.id}_person" }) +
" " + person.name
), :class => 'checkbox') + tag(:br)
end
To find a better solution, we should rethink what the helper should be responsible for. Helpers are useful when they hide complexity that isn’t relevant to the template. Looking at this helper method, I see that it’s useful to hide away check_box_tag with all those params. But the label, the break, and even the person’s name could all be in the HTML and the template would be clearer. Let’s do that.
Here’s the new helper. Now it only produces the checkbox.
def add_person_to_project_check_box(person, company)
check_box_tag("people_ids[]", person.id, false, { :class => "company_#{company.id}_person" })
end
And here’s the updated template code, with the label, person name, and break moved over.
<% people_without_access_from(company).each do |person| %>
<label class="checkbox">
<%= add_person_to_project_check_box(person, company) %> <%= person.name %>
</label><br />
<% end %>
Now that’s a lot easier to read. Generally speaking, it’s a good idea to keep your HTML in your templates and out of your helpers. Helpers are useful when they hide implementation details that are irrelevant to your template, or when they allow you to abstract common template code to avoid repetition. If you find yourself generating a lot of HTML in a helper, think twice and try to keep as much HTML in your template as possible.
The grey buttons toggle overlaying panels
