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.

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

Add unordered_list helper #5637

Closed
wants to merge 1 commit into from
Closed

Add unordered_list helper #5637

wants to merge 1 commit into from

Conversation

@jgaskins
Copy link
Contributor

@jgaskins jgaskins commented Mar 28, 2012

In an effort to reduce some common boilerplate for templates, I present to you an unordered_list view helper that will allow us to reduce this:

<ul class="items">
  <% @items.each do |item| %>
    <li class="item"><%= link_to item.name, item %></li>
  <% end %>
</ul>

down to this:

<%= unordered_list(@items) { |item| link_to item.name, item } %>
@drogus
Copy link
Member

@drogus drogus commented Mar 28, 2012

thanks for the patch, but I'm -1 for this. It will work only for simplest cases, but if you for example need anything more than just classes on ul and li elements you will have to revert to regular HTML or extend the helper, which will probably make it complicated and will make code harder to read.

@jgaskins
Copy link
Contributor Author

@jgaskins jgaskins commented Mar 28, 2012

I agree that it's primarily for simple cases. The idea was to make those simple cases even simpler because I find myself writing the simple cases often enough. :-)

I didn't think about this at the time, but I think doing something like this:

<%= unordered_list @items do |item| %>
  <%= markup_for_item %>
  <%= more_markup_for_item %>
<% end %>

would be a good use case for this — unfortunately in it's current form, it won't do that so I'd have to keep going with it. I just thought it'd be a good idea to get rid of some of the repetition of writing loops over collections.

EDIT: Okay, I lied. The above code totally works. I had a typo in my view code. :-)

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 28, 2012

I'm 👎 with this one too. You already can do this:

content_tag_for(:li, @items, :class => "item") do |item|
  <%= link_to item.name, item %>
end
@drogus
Copy link
Member

@drogus drogus commented Mar 28, 2012

@jgaskins you could just wrap @rafaelfranca's code in a simple helper to simplify your views

I'm closing this.

@drogus drogus closed this Mar 28, 2012
@jgaskins
Copy link
Contributor Author

@jgaskins jgaskins commented Mar 29, 2012

@rafaelfranca Sort of. You'd still need to wrap those lis within a ul. Since the only elements within a ul should be lis, I figured this was a good abstraction by doing this at the list level. I did forget, however, that content_tag_for would iterate over a collection … or I would've used it in my code. :-)

Anyway, I appreciate you guys taking the time to look at this. I'll continue looking for other places to help out.

@drogus
Copy link
Member

@drogus drogus commented Mar 29, 2012

Anyway, I appreciate you guys taking the time to look at this. I'll continue looking for other places to help out.

Cool! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.