Proposal: Allow rendering templates

[twe4ked]

Just had a quick play with the scenario rendering to get it to work without needing a partial.

The following change allows this to work:

diff --git a/app/views/guide/scenarios/_scenario.html.erb b/app/views/guide/scenarios/_scenario.html.erb
index a859593..a5ad77e 100644
--- a/app/views/guide/scenarios/_scenario.html.erb
+++ b/app/views/guide/scenarios/_scenario.html.erb
@@ -8,8 +8,10 @@
           <pre><%= render "#{view.template}.#{view.format}",
                           Guide.configuration.local_variable_for_view_model => view.view_model %></pre>
         <% elsif view.format == 'html' %>
-          <%= render  view.template,
-                      Guide.configuration.local_variable_for_view_model => view.view_model %>
+          <%= render  template: view.template,
+                      locals: {
+                        Guide.configuration.local_variable_for_view_model => view.view_model
+                      } %>
         <% else %>
           <%= render  "#{view.template}.#{view.format}",
                       Guide.configuration.local_variable_for_view_model => view.view_model %>

I noticed that Guide::Structure#template is basically an alias for Guide::Structure#partial. I wonder if we could change it so template would refer to a template and partial could refer to a partial.

As part of this change it would be nice to make local_variable_for_view_model configurable per structure. That was we're not tied to using the same local variable name for the whole system.

Thoughts?

[mariovisic]

I like it

[jiexinhuang]

I like the idea to render view template directly, very handy when the template is small.

In terms of local_variable_for_view_model, I don't think it is a problem. Consider presenter as generic context of template, you can nest meaningful names under it.

presenter = {
  user: {
    id: 1,
    name: 'Bob'
  }
}

compare to:

user = user: {
  id: 1,
  name: 'Bob'
}

Thoughts?

[twe4ked]

@jiexinhuang I'm working on something where I have a few presenters being created as helper methods from the controller so they need different names.

[bigfive]

You can do: render 'something', presenter: presenter_from_helper_1 ... render 'something_else', presenter: another_presenter_from_a_helper

FWIW I like the enforcement of having one exposed local variable called 'presenter' per thing (view model? cell? :insert-correct-word-here:)

[twe4ked]

True, but then you need to override the render calls. I'm happy to let this one go. I'm just not a huge fan of having our documentation controlling the way we structure our code down to the variable names.

[bigfive]

I would argue that our documentation takes advantage of the way we have decided to structure out code. But yeah, theres some pretty tight coupling now.

👍 to the main proposal btw

[zubin]

Great idea! 👍

[jiexinhuang]

instead of using helper method, what if you pass all presenters into the presenter?

presenter = OpenStruct.new(
  first_presenter: presenter_one,
  second_presenter: presenter_two
)

thoughts?

[twe4ked]

In this case the two presenters are for completely different pages.

[lukearndt]

I actually like that local_variable_for_view_model set on a per-app basis because it forces the host app to be consistent. I don't see a valid reason for having different local variable names for templates to access their view model, that sounds like it would just be confusing.

[twe4ked]

I'm happy to let this one go.

[lukearndt]

Regarding using template and partial differently, I think I'd rather not. partial largely only there because the Market app currently uses partial everywhere.

The core idea of this, though? 💯

[twe4ked]

How would you propose it works then?

[lukearndt]

The code that you showed seems legit.

What will happen if the format is not html?

[twe4ked]

I believe it would need to be render :partial => 'foo' for the partials.

[lukearndt]

Oh, so render :template => file doesn't work for partials?

[twe4ked]

No, that's why I was thinking we should go with the two methods.

[lukearndt]

@twe4ked and I just had a chat about it on Slack. Here's what came out of that:

Deciding whether to render using :template => location on the basis of whether the template or partial methods are nil / contain values isn't great. What we could try instead:

Structure definition that uses partials:

def template
  { :partial => 'location' }
end

Structure definition that uses top level rails views:

def template
  { :template => 'location' }  
end

Assuming that Rails will render just fine with render :partial => location, this should take away the need to put that decision into _scenario.html.erb:

<%= render  view.template,
            :locals => { Guide.configuration.local_variable_for_view_model => view.view_model } %>

If Rails doesn't let us use a hash syntax to render partials, we might need to do this:

Structure definition:

def template
  'such/location/wow'
end

def render_as_top_level_template? #name pending
  true #(defaults to false in `structure.rb`)
end

_scenario.html.erb:

<% if view.render_as_top_level_template? %>
  <%= render  :template => view.template,
              :locals => { Guide.configuration.local_variable_for_view_model => view.view_model } %>
<% else %>
  <%= render  view.template,
              Guide.configuration.local_variable_for_view_model => view.view_model %>
<% end %>

[bigfive]

I think you'll need to do:

<%= render view.template.merge( :locals => { Guide.configuration.local_variable_for_view_model => view.view_model }) %>

That seems much more elegant than overriding a render_as_top_level_template? method

[twe4ked]

@bigfive yeah, that's what we're aiming for.