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

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

Open
lelelelemon opened this issue Aug 23, 2017 · 4 comments

Comments

@lelelelemon
Copy link
Collaborator

lelelelemon commented Aug 23, 2017

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
Copy link
Owner

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

@lelelelemon
Copy link
Collaborator Author

Thanks~ 👍

@drhenner
Copy link
Owner

drhenner commented Sep 3, 2017

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
Copy link
Owner

drhenner commented Sep 3, 2017

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.

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

No branches or pull requests

2 participants