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.

Sign up for GitHub

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

Jump to bottom

Make possible rendering of a block with a layout and a collection. #13447

Closed
wants to merge 1 commit into from
Closed

Make possible rendering of a block with a layout and a collection. #13447

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions actionview/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
* Make possible rendering of a block with a layout and a collection

Fixes #13354.

*Alex Tsukernik*

* The `video_tag` helper accepts a number as `:size`

The `:size` option of the `video_tag` helper now can be specified
Expand Down
2 changes: 1 addition & 1 deletion actionview/lib/action_view/helpers/rendering_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ def _layout_for(*args, &block)
if block && !name.is_a?(Symbol)
capture(*args, &block)
else
super
super(name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When calling super without arguments, ruby will automatically provide the arguments originally sent to the method.
So, as the argument isn't modified here, explicitely specifying name doesn't seem useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super has a different signature def _layout_for(name=nil)

end
end
end
Expand Down
7 changes: 5 additions & 2 deletions actionview/lib/action_view/renderer/partial_renderer.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ def collection_with_template
view, locals, template = @view, @locals, @template
as, counter = @variable, @variable_counter

if layout = @options[:layout]
if !@block && (layout = @options[:layout])
layout = find_template(layout, @template_keys)
end

Expand All @@ -396,7 +396,10 @@ def collection_with_template
locals[as] = object
locals[counter] = (index += 1)

content = template.render(view, locals)
content = template.render(view, locals) do |*name|
name << name.extract_options!.merge!(locals)
view._layout_for(*name, &@block)
end
content = layout.render(view, locals) { content } if layout
content
end
Expand Down
9 changes: 9 additions & 0 deletions actionview/test/template/render_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,15 @@ def test_render_layout_with_block_and_yield_with_params
@view.render(:layout => "layouts/yield_with_params") { |param| "#{param} Content from block!" }
end

def test_render_layout_with_collection_and_block_and_yield_with_params
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could make this a bit more readable without sacraficing line count:

args     = {layout: "layouts/yield_with_params", collection: %w[item1 item2], as: :item }
actual   = @view.render(args) do |param, locals|
              "#{param} Content from block! #{locals[:item]}:#{locals[:item_counter]}"
            end
expected = %(Yield! Content from block! item1:0\nYield! Content from block! item2:1\n)
assert_equal expected, actual

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test started as a copy, they all look like this, I think it's readable enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was hard for me to read, even if the others are hard to read no reason we can't make this one better. At the end of the day it won't block the PR but are you happy with the Rails codebase being just good enough ? 😄

assert_equal %(Yield! Content from block! item1:0\nYield! Content from block! item2:1\n),
@view.render(:layout => "layouts/yield_with_params",
collection: %w[item1 item2],
as: :item ) { |param, locals|
"#{param} Content from block! #{locals[:item]}:#{locals[:item_counter]}"
}
end

def test_render_layout_with_block_which_renders_another_partial_and_yields
assert_equal %(partial html\nContent from block!\n),
@view.render(:layout => "layouts/partial_and_yield") { "Content from block!" }
Expand Down