Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Is there a better way to implement my carousel in less lines?

<div id="myCarousel" class="carousel slide">
  <div class="carousel-inner">
    <% @photos.each_with_index do |photo, index| %>
      <% if index == 0 %>
      <div class="item active">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
        <div class="container">
          <div class="carousel-caption">
            <h1>TITLE</h1>
          </div>
        </div>
      </div>
      <% end %>
      <% if index == 1 %>
      <div class="item">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
      </div>
      <% end %>
      <% if index == 2 %>
      <div class="item">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
      </div>
      <% end %>
      <% if index == 3 %>
      <div class="item">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
      </div>
      <% end %>
      <% if index == 4 %>
      <div class="item">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
      </div>
      <% end %>
      <% if index == 5 %>
      <div class="item">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
      </div>
      <% end %>
    <% end %>
  </div>
</div><!-- /.carousel -->
share|improve this question

2 Answers 2

up vote 4 down vote accepted

Get rid of unnecessary code:

<div id="myCarousel" class="carousel slide">
  <div class="carousel-inner">
    <% @photos.each_with_index do |photo, index| %>
      <% if index.zero? %>
      <div class="item active">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
        <div class="container">
          <div class="carousel-caption">
            <h1>TITLE</h1>
          </div>
        </div>
      </div>
      <% else %>
      <div class="item">
        <img src="<%= photo.image_url(:large) %>" alt="<%= photo.title %>">
      </div>
      <% end %>
    <% end %>
  </div>
</div>

Move stuff to helpers:

module PhotoHelpers
  def slider_image(photo)
    image_tag photo.image_url(:large), alt: photo.title
  end
end

<div id="myCarousel" class="carousel slide">
  <div class="carousel-inner">
    <% @photos.each_with_index do |photo, index| %>
      <% if index.zero? %>
        <div class="item active">
          <%= slider_image photo %>
          <div class="container">
            <div class="carousel-caption">
              <h1>TITLE</h1>
            </div>
          </div>
        </div>
      <% else %>
        <div class="item">
          <%= slider_image photo %>
        </div>
      <% end %>
    <% end %>
  </div>
</div>
share|improve this answer
    
Perfect - thank you. –  thmsmxwll Jan 21 '13 at 10:54
    
any reason for not indenting conditionals? –  tokland Jan 21 '13 at 12:18
    
@tokland no reason, it wasn't originally indented. I've updated. –  NARKOZ Jan 21 '13 at 12:31

Along with @NARKOZ's excellent answer, consder converting your template to haml:

#myCarousel.carousel.slide
  .carousel-inner
    - @photos.each_with_index do |photo, index|
      - if index.zero?
        .item.active
          %img{:alt => photo.title, :src => photo.image_url(:large)}/
          .container
            .carousel-caption
              %h1 TITLE
      - else
        .item
          %img{:alt => photo.title, :src => photo.image_url(:large)}/

All you have to do is name your template .haml instead of .erb, and include the haml gem in your Gemfile.

The haml gem comes with a conversion script, html2haml, which will convert your erb to haml.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.