Patch #11704

Avoid the use of tag("...", "...", true) in layout

Added by Jean-Baptiste Barth over 2 years ago. Updated over 2 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:Code cleanup/refactoring
Target version:2.1.0

Description

Here's a very simplified version of Redmine base layout :

<html>
  <body>
    <%= tag('div', {:id => 'main', :class => (sidebar_content? ? '' : 'nosidebar')}, true) %>
      <div id="sidebar">
        <%= yield :sidebar %>
      </div>
      <div id="content">
        <%= yield %>
      </div>
    </div>
  </body>
</html>

There's a "tag" helper, processed with ERB, to display "div#main", and it's closed with standard html in the end.

It could be considered as bad practice, but the main problem is that you cannot parse the layout correctly before it's fully processed by ERB. Hence gems that hook in the rendering process don't work correctly for the layout partial. It's the case of deface (https://github.com/railsdog/deface) for instance, a gem I use these days so that I don't need 50 more view hooks in core.

1st solution : use inline erb just for the class

i.e. replace the tag line with :

  <div id="main" class="<%= sidebar_content? ? '' : 'nosidebar' %>">

2nd solution : use content_tag helper

i.e. something like that :

<html>
  <body>
    <%= content_tag 'div', :id => 'main', :class => (sidebar_content? ? '' : 'nosidebar') do %>
      <div id="sidebar">
        <%= yield :sidebar %>
      </div>
      <div id="content">
        <%= yield %>
      </div>
    <% end %>
  </body>
</html>

I understand this isn't a problem for redmine core for now, but such a change would ease plugin development a lot and avoid overriding the layout/base partial.

Any thought on this is welcome.

Associated revisions

Revision 10237
Added by Jean-Philippe Lang over 2 years ago

Don't use tag helper in layout (#11704).

History

#1 Updated by Jean-Philippe Lang over 2 years ago

  • Target version set to 2.1.0

#2 Updated by Jean-Philippe Lang over 2 years ago

  • Status changed from New to Closed

Solution 1 committed in r10237.

#3 Updated by Jean-Baptiste Barth over 2 years ago

Thanks a lot :)

Also available in: Atom PDF