Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
Contributor

bughit commented Dec 22, 2013

Fixes #13354.

Contributor

bughit commented Dec 22, 2013

@rafaelfranca

both render_partial and now collection_with_template render the block via view._layout_for which allows the layout to render content_for captures instead of/in addition to the block, by passing a symbol to yield

I am not sure this make sense. In a controller layout scenario the template is rendered first and is therefore able to supply content_for to the layout. Here, the block is rendered on demand, and is not able to supply content to the the layout. So what value does _layout_for provide?

Also the docs already state that the layout is able to pass args to the block, so I guess rendering the block first is not an option.

So perhaps it makes sense to avoid _layout_for for block rendering.

Contributor

bughit commented Dec 22, 2013

well, there are tests that depend on _layout_for, so never mind

@schneems schneems commented on the diff Dec 22, 2013

actionview/test/template/render_test.rb
@@ -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
@schneems

schneems Dec 22, 2013

Member

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
@bughit

bughit Dec 22, 2013

Contributor

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

@schneems

schneems Dec 22, 2013

Member

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 ? 😄

@dmathieu dmathieu commented on the diff Dec 24, 2013

actionview/lib/action_view/helpers/rendering_helper.rb
@@ -82,7 +82,7 @@ def _layout_for(*args, &block)
if block && !name.is_a?(Symbol)
capture(*args, &block)
else
- super
+ super(name)
@dmathieu

dmathieu Dec 24, 2013

Contributor

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.

@bughit

bughit Dec 24, 2013

Contributor

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

Contributor

bughit commented Dec 24, 2013

going to make a change later, don't merge

Contributor

bughit commented Dec 24, 2013

I am thinking of the following change:

        content = template.render(view, locals) do |*name|
          if view._yield_to_block?(name.first, @block)
            name << name.extract_options!.merge!(locals)
            view.capture(*name, &@block)
          else
            view._layout_for(*name)
          end
        end

      def _layout_for(*args, &block)
        name = args.first

        if _yield_to_block?(name, block)
          capture(*args, &block)
        else
          super(name)
        end
      end

      def _yield_to_block?(first_yield_arg, block)
        block && !first_yield_arg.is_a?(Symbol)
      end

the idea is to only do locals merging if you know you are going to be capturing the block, but that decision is made in _layout_for, so I factored it out. Should I go with this?

Contributor

bughit commented Dec 28, 2013

@rafaelfranca can you please review this and the proposed changes above

Contributor

bughit commented Jan 3, 2014

@rafaelfranca - or suggest someone else

@rafaelfranca rafaelfranca was assigned Jan 3, 2014

Owner

rafaelfranca commented Jan 3, 2014

ack. Assigned to me and I'll take a look tomorrow

Contributor

bughit commented Apr 9, 2014

@robin850 robin850 added the actionview label Apr 22, 2014

sbonami commented Sep 17, 2015

@rafaelfranca I believe this is still an issue. Anything I can do to get this bumped?

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