drhenner / ror_ecommerce

Ruby on Rails Ecommerce platform, perfect for your small business solution.
www.ror-e.com
MIT License
1.21k stars 409 forks source link

some html label generate code could be changed in products/index.html.erb #185

Open lelelelemon opened 7 years ago

lelelelemon commented 7 years ago

When rendering the products, link_to, image_tag and some other functions are used to generate corresponding label, however they might be expensive, and the contents generated actually have a lot of things in common. It's possible to use string replace to achieve this function. Also, when the product.status_stock is 'low stock', the same ribbon image will be rendered, so there is no need to generate the image_tag every time, we can put it out of the loop and reuse it when necessary. Here is my patch, it somewhat make the code a little bit hard to read, but it indeed improve performance, especially when one page have a lot of products to show.

diff --git a/app/views/products/index.html.erb b/app/views/products/index.html.erb
index 834be42..b0e968d 100644
--- a/app/views/products/index.html.erb
+++ b/app/views/products/index.html.erb
@@ -7,17 +7,18 @@
Here is my patch  
   <div id='interesting_items' >
     <ul id='interesting_items-list'>
+      <% low_stock = image_tag("ribbons/low_stock.png",
+                                 width: 63, height: 62,
+                                 class: 'upper_left_overlay' ) %>
+      <% no_image = image_tag("no_image_medium.jpg") %>
+      <% image = "<img src = 'image_path' alt = ''/>" %>
       <% @products.each_with_index do |product, i| %>
         <li id='interesting_product_<%= i %>' class=''>
           <div class='interesting_items-image'>
-
-            <%= link_to product_path(product), title: product.name do %>
-
+            <a title = '<%= product.name %>' href = '<%= product_path(product)%>'>
               <div class='no-hover-show'>
                 <% if product.hero_variant.try(:low_stock?) %>
-                  <%= image_tag("ribbons/#{product.stock_status}.png",
-                                 width: 63, height: 62,
-                                 class: 'upper_left_overlay' ) %>
+                  <%= low_stock %>
                 <% end %>
                 <div class='hover-details unfade'>
                   <p class='bottom-hover-details'>
@@ -29,8 +30,15 @@
                   </p>
                 </div>
               </div>
-              <%= link_to image_tag(product.featured_image(:medium)), product_path(product), title: product.name, class:  'clearfix' %>
-            <% end %>
+              <a title = '<%= product.name %>' class:  'clearfix' href = '<%= product_path(product)%>' >
+               <% if product.featured_image(:medium) != "no_image_medium.jpg" %>
+                   <%= image.gsub('image_path', product.featured_image(:medium)).html_safe %>
+               <% else %>
+                   <%= no_image %>
+               <% end %>
+              </a>
+            </a>
           </div>

           <div class='interesting_items-details'><%= product.name %><br/>
drhenner commented 7 years ago

I'll try to look at this after I have more sleep.

lelelelemon commented 7 years ago

Thanks~ 👍

drhenner commented 7 years ago

I'm going to do a bit of changing to this. I think adding this as a helper method might work best.

Also there is a low stock banner & out_of_stock banner. Each needs to be displayed when needed.

drhenner commented 7 years ago

Can you show me the benchmarks you saw?

The difference aren't obviously much faster.

My code does some string interpolation Multiple times but that is super fast (plus checks for out of stock vs just low stock which is needed).

Then my code is using rails link_to vs <a> tag which should be minimally better.

I have a feeling your big speed improvement is because you aren't checking for the needed out_of_stock product.stock_status. If you don't need that then removing is fine but I'm leaving it for now.