ror_ecommerce icon indicating copy to clipboard operation
ror_ecommerce copied to clipboard

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

Open lelelelemon opened this issue 8 years ago • 4 comments
trafficstars

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/>

lelelelemon avatar Aug 23 '17 03:08 lelelelemon

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

drhenner avatar Aug 23 '17 07:08 drhenner

Thanks~ 👍

lelelelemon avatar Aug 23 '17 07:08 lelelelemon

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 avatar Sep 03 '17 06:09 drhenner

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.

drhenner avatar Sep 03 '17 07:09 drhenner