Make `content_tag_for` and `div_for` accepts the array of records #2816

Merged
merged 1 commit into from Sep 4, 2011

Projects

None yet

4 participants

@sikachu
Member
sikachu commented Sep 2, 2011

So instead of having to do this:

   @items.each do |item|
     content_tag_for(:li, item) do
        Title: <%= item.title %>
     end
   end

You can now do this:

   content_tag_for(:li, @items) do |item|
     Title: <%= item.title %>
   end

(Same goes for the div_for)

@vijaydev vijaydev commented on an outdated diff Sep 3, 2011
actionpack/lib/action_view/helpers/record_tag_helper.rb
@@ -17,6 +17,19 @@ module ActionView
#
# <div id="person_123" class="person foo"> Joe Bloggs </div>
#
+ # You can also passed an array of Active Record objects, which will then
@vijaydev
vijaydev Sep 3, 2011 Ruby on Rails member

s/passed/pass

@vijaydev vijaydev commented on an outdated diff Sep 3, 2011
railties/guides/source/action_view_overview.textile
@@ -454,6 +454,83 @@ input("post", "title") # =>
<input id="post_title" name="post[title]" size="30" type="text" value="Hello World" />
</ruby>
+h4. RecordTagHelper
+
+This module provides methods for generating a container tag, such as a +<div>+, for your record. This this the recommended way of creating a container for render your Active Record object, as it adds an appropriate class and id attributes to that container. You can then refer to those containers easily by following the convention, instead of having to think about which class or id attribute you should use.
@vijaydev
vijaydev Sep 3, 2011 Ruby on Rails member

s/This this/This is

@vijaydev vijaydev commented on an outdated diff Sep 3, 2011
railties/guides/source/action_view_overview.textile
+
+<ruby>
+<%= content_tag_for(:tr, @post) do %>
+ <td><%= @post.title %></td>
+<% end %>
+</ruby>
+
+This will generate this HTML output:
+
+<html>
+<tr id="post_1234" class="post">
+ <td>Hello World!</td>
+</tr>
+</html>
+
+You can also supply a HTML attributes as an additional option hash. For example:
@vijaydev
vijaydev Sep 3, 2011 Ruby on Rails member

remove the 'a'

@vijaydev vijaydev commented on an outdated diff Sep 3, 2011
railties/guides/source/action_view_overview.textile
+
+<ruby>
+<%= content_tag_for(:tr, @post, :class => "frontpage") do %>
+ <td><%= @post.title %></td>
+<% end %>
+</ruby>
+
+Will generate this HTML output:
+
+<html>
+<tr id="post_1234" class="post frontpage">
+ <td>Hello World!</td>
+</tr>
+</html>
+
+Additionaly, you can pass a collection of Active Record objects. This method will loops through your objects and create a container for each of them. For example, given +@posts+ is an array of two +Post+ objects:
@vijaydev
vijaydev Sep 3, 2011 Ruby on Rails member

s/Additionaly/Additionally. Though I prefer, "You can also pass a collection of .. "

@sikachu
Member
sikachu commented Sep 3, 2011

@vijaydev pull request updated as requested.

@sikachu
Member
sikachu commented Sep 3, 2011

@josevalim bro, can I have a review of this commit?

@josevalim josevalim and 1 other commented on an outdated diff Sep 3, 2011
actionpack/lib/action_view/helpers/record_tag_helper.rb
@@ -53,10 +81,20 @@ module ActionView
# <li id="person_123" class="person bar">...
#
def content_tag_for(tag_name, record, prefix = nil, options = nil, &block)
- options, prefix = prefix, nil if prefix.is_a?(Hash)
- options ||= {}
- options.merge!({ :class => "#{dom_class(record, prefix)} #{options[:class]}".strip, :id => dom_id(record, prefix) })
- content_tag(tag_name, options, &block)
+ if record.respond_to?(:to_ary)
+ record.map do |single_record|
@josevalim
josevalim Sep 3, 2011 Ruby on Rails member

Shouldn't this be record.to_ary.map ? I don't think implementing to_ary means it implements all Enumerable methods.

@sikachu
sikachu Sep 3, 2011 Ruby on Rails member

Yep, I totally agree on this one.

@josevalim josevalim and 1 other commented on an outdated diff Sep 3, 2011
actionpack/lib/action_view/helpers/record_tag_helper.rb
@@ -53,10 +81,20 @@ module ActionView
# <li id="person_123" class="person bar">...
#
def content_tag_for(tag_name, record, prefix = nil, options = nil, &block)
@josevalim
josevalim Sep 3, 2011 Ruby on Rails member

For readability, maybe we could break this method into two? content_tag_for and content_tag_for_each?

@sikachu
sikachu Sep 3, 2011 Ruby on Rails member

What do you mean?

  1. Two private methods that performs on multiple/single objects?
  2. Retain the old method, but add another method name content_tag_for_each?
@josevalim
josevalim Sep 3, 2011 Ruby on Rails member

I had in mind something like this:

def content_tag_for(tag_name, record, *args, &block)
  if record.respond_to?(:to_ary)
    record.to_ary.map do |single_record|
      capture { content_tag_for_each(tag_name, single_record, *args, &block) }
    end.join("\n")
  else
    content_tag_for_each(tag_name, record, *args, &block)
  end
end

And the content_tag_for_each would do the magic (and would likely be a private method).

@sikachu
sikachu Sep 3, 2011 Ruby on Rails member

Then we're on the same page. Thanks for suggestion dude, changing it now.

@sikachu sikachu Make `content_tag_for` and `div_for` accepts the array of records
So instead of having to do this:

   @items.each do |item|
     content_tag_for(:li, item) do
        Title: <%= item.title %>
     end
   end

You can now do this:

   content_tag_for(:li, @items) do |item|
     Title: <%= item.title %>
   end
b84cee0
@sikachu
Member
sikachu commented Sep 4, 2011

@josevalim pull request updated.

I think it's better to change the method signature a bit too, to make sure what to expected from the variable.

@josevalim josevalim merged commit 036a250 into rails:master Sep 4, 2011
@josevalim
Member

Perfect pull request: docs + code + tests. Merged.

@spastorino
Member

NICE CATCH!!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment