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

Fix blurry product/collection grid images #2277

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

marija-scuric
Copy link

@marija-scuric marija-scuric commented Feb 2, 2023

PR Summary:

Fixes blurry card images by including dynamic intrinsic size calculation for the rendered images in grids.

Why are these changes introduced?

Fixes #763 | #532

What approach did you take?

Based on @hiroakiito2002's suggested solution, this change introduces a variable to store the column values based on the theme editor settings selection for the number of columns for desktop and mobile rather than the current fixed value in the calculation, which can cause card images to be displayed in lower quality.

For example, the featured collection section has the card image size calculated based on 4 product cards being displayed, however a row can contain anywhere between 1 and 5 products, which results in lower quality images when there are fewer than 4 products.

This way, the calculation for product and collection cards is based on dynamic input. There's a reference to the column values when rendering snippets in section files (collection, collection list, search, product recommendations).

Other considerations

In some cases where uploaded images are very large, the load time may be slightly increased to accommodate the larger image size.

Visual impact on existing themes

For some merchants, rendered card images will be in higher quality.

Testing steps/scenarios

  • Step 1 - Home page: Collection-list.liquid section, make sure to test different number of columns on mobile and desktop and see if the intrinsic size updates based on the different selection
  • Step 2 - Home page: Featured-collection.liquid section, make sure to test different number of columns on mobile and desktop and see if the intrinsic size updates based on the different selection
  • Step 3 - Product page: Recommended-products.liquid section, make sure to test different number of columns on mobile and desktop and see if the intrinsic size updates based on the different selection
  • Step 4 - Product grid of the Collection page, make sure to test different number of columns on mobile and desktop and see if the intrinsic size updates based on the different selection
  • Step 5 - Collection grid of the Collection list page, make sure to test different number of columns on mobile and desktop and see if the intrinsic size updates based on the different selection
  • Step 6 - Search results page, make sure to test different number of columns on mobile and desktop and see if the intrinsic size updates based on the different selection, searching for a should result in a blog page, products and a page in results

Demo links

Checklist

@eugenekasimov eugenekasimov self-requested a review March 1, 2023 22:19
Copy link
Contributor

@eugenekasimov eugenekasimov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I left a couple of minor comments.

sections/collection-list.liquid Outdated Show resolved Hide resolved
sections/main-list-collections.liquid Outdated Show resolved Hide resolved
Copy link
Contributor

@ludoboludo ludoboludo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple issues I noted:

  • With the collage section. We're loading the product and collection card without passing any columns_desktop value. Which means we're going to load the media like there was only 1 column which isn't great. We're going to need a bit of logic there I think to figure out the amount of column it should be using. Collage is a 3 columns grid where the card can take either 2/3, or 1/3, or 3/3 if there is only one block. Then on mobile depending on the amount of block mobile layout chosen it can be 1 or two columns.
  • With complementary products. We are currently not passing in any column amount. This one is tough cause there isn't any columns but it something coming up pretty small and will depend a bit of the size of the media column of the product page. If that column is small, the complementary product cards will use bigger images than when the media column is large.

Comment on lines 36 to 37
assign desktop = columns_desktop | at_least: 1
assign mobile = columns_mobile | at_least: 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we could use better descriptive naming I think. Those names don't convey clearly what the value is tied to. Maybe columns_amount_desktop 🤷 or something else

Comment on lines 32 to 33
assign desktop = columns_desktop | at_least: 1
assign mobile = columns_mobile | at_least: 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here for the naming. Having something a bit more descriptive so it's easier to read would be great.

@Roi-Arthur
Copy link
Contributor

Roi-Arthur commented Apr 4, 2023

@ludoboludo

  • With the collage section.

I think this would cover the logic for how many columns to pass into the collage section blocks (both product & collection):

{% liquid
  if section.blocks.size == 1
    assign columns_amount_desktop = 1
  elsif forloop.first and section.settings.desktop_layout == 'left' or forloop.last and section.settings.desktop_layout == 'right'
    assign columns_amount_desktop = 1.5
  else
    assign columns_amount_desktop = 3
  endif
  

  if section.settings.mobile_layout == 'column' or section.blocks.size == 1
    assign columns_amount_mobile = 1
  else
    if section.blocks.size == 3
      if forloop.first and section.settings.desktop_layout == 'left' or forloop.last and section.settings.desktop_layout == 'right'
        assign columns_amount_mobile = 1
      else 
        assign columns_amount_mobile = 2
      endif
    else
      assign columns_amount_mobile = 2
    endif
  endif
%}

For desktop:

  • Using only 1 block we have 1 column
  • Using more than 1 block, the desktop_layout setting ('left' or 'right') dictates which bock will be big or small:
    • The first block when 'left' or the last block when 'right' is big (3 columns times 2, so I think we want to divide by 1.5)
    • The other blocks are small (3 columns)

For mobile:

  • If the layout is 'column' or if we have only one block we have only 1 column
  • If the layout is 'collage', we're back to counting blocks and looking at desktop_layout:
    • The first block when desktop_layout is 'left' or the last block when desktop_layout is 'right' is big (1 column)
    • The other blocks are small (2 columns)

  • With complementary products

The image always takes 20% of its container, so I would naively multiply the number of columns the container takes by 5?
Something like this:

{% liquid 
  if section.settings.media_size == 'small'
    assign column_desktop = 7.5
  elsif section.settings.media_size == 'medium'
    assign column_desktop = 10
  else
    assign column_desktop = 15
  endif
  assign column_mobile = 5
%} 

For desktop:

  • If the media column is 'small', the details are 2 out of 3 columns, so we would divide by 1.5 * 5 = 7.5
  • If the media column is 'medium', the details are 1 out of 2 columns, so we would divide by 2 * 5 = 10
  • If the media column is 'large', the details are 1 out of 3 columns, so we would divide by 3 * 5 = 15

For mobile the media size doesn't matter and we always use 20% of 1 column, so we want to divide by 5

@@ -4,7 +4,8 @@
Accepts:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To reply to your comment @Roi-Arthur

I think we can change the logic a little. the desktop variable is assigned at_least: 1 so in some cases we don't need to assign anything in the collage file.
Here is the mobile logic you have but a bit reworked, I'd have to actually test it to make sure the and is read properly:

{% liquid 
  unless section.settings.mobile_layout == 'column' or section.block.size == 1
    if section.settings.mobile_layout == 'collage'
      if section.blocks.size == 3 and forloop.first and section.settings.desktop_layout == left or forloop.last and section.settings.desktop_layout == right
        assign columns_amount_mobile = 1
      else
        assign columns_amount_mobile = 2
      endif
    endif
  endunless
%}

For the complementary products it's a bit trickier 🤔 I think from what I can see their size is going from 64px wide up to potentially 140px when it's on a product with no image. And 118px on a product with images.

Maybe we could do something a bit simpler that isn't quite perfect but good enough. Something like defaulting to 10 for desktop and 5 for mobile 👍 This way we don't go into logic that's a bit hard to understand.

What do you think?

Copy link
Contributor

@Roi-Arthur Roi-Arthur Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the desktop variable is assigned at_least: 1 so in some cases we don't need to assign anything in the collage file.

I did think of this after the fact and was wondering what's preferable: one the one hand we have more liquid but it's very readable and you know exactly what you're passing to the snippet so might be more user friendly, especially for people who are getting started. On the other we can definitely remove all cases where the assign is for 1 in terms of efficiency.

I'd have to actually test it to make sure the and is read properly:

That was also my concern 😅
Haven't tried any of this yet; was just a draft starting from what sounded logical.

For the complementary products it's a bit trickier

Totally up for something that does 80% of the job if lets us sidestep making more calculations (especially since they do feel a bit arbitrary)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out that you cannot chain Liquid operators the way you want; specifically, operators are evaluated from left to right, with no possibility to use parentheses to create "logic blocks". So with that in mind we can't do the long checks I had previously included.

As a result this is what I tested that seems to be working to declare variables before the card snippets:

{% liquid
  unless section.blocks.size == 1
    if forloop.first and section.settings.desktop_layout == 'left'
      assign columns_amount_desktop = 1.5
    elsif forloop.last and section.settings.desktop_layout == 'right'
      assign columns_amount_desktop = 1.5
    else
      assign columns_amount_desktop = 3
    endif
  endunless

  assign columns_amount_mobile = 1
  unless section.settings.mobile_layout == 'column' or section.blocks.size == 1
    unless section.blocks.size == 3 and forloop.first and section.settings.desktop_layout == 'left'
      unless section.blocks.size == 3 and forloop.last and section.settings.desktop_layout == 'right'
        assign columns_amount_mobile = 2
      endunless
    endunless
  endunless
%}

Note the assign columns_amount_mobile = 1 before the mobile assignment, as it appears that the variable value carried forward through the next iterations of the loop? At least that was my conclusion as I was ending up with "columns_amount_mobile = 2" in the last loop even when with desktop_layout = 'right' and this fixed it.

Copy link
Contributor

@ludoboludo ludoboludo May 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a correction, you mentioned that operators are evaluated from left to right which isn't right. It's the opposite (source).

Roi-Arthur and others added 7 commits April 14, 2023 17:33
* [Motion] Use rootMargin instread of threshold to trigger slide in animations (#2517)

* Use rootMargin instread of threshold to trigger slide in animations

* address review comment, remove threshold

* Add conditional around data-cascade attribute (#2532)

* [Motion] No animation on added/edited section (#2502)

---------

Co-authored-by: Ludo <[email protected]>
@Roi-Arthur
Copy link
Contributor

Here we go, I think we're covered now. The current changes adjust:

  • The Collage section loads images with the logic we've mentioned before, taking into account the fact that we can't chain operators exactly like we want (hence a nested unless block)
  • Make column_amounts more explicit in our card-product and card-collection snippets
  • Adjust references to those snippets in main-product.liquid and cart-drawer.liquid snippets
    • In main-product.liquid, for the complementary product blocks I added the defaults we talked about (10 for desktop, 5 for mobile)
    • In cart-drawer.liquid, since we load the image for the line items, I've added defaults based on the size at which we load things, which appears to be a constant 150px (both desktop and mobile). As such I've made the defaults 5 for desktop and 2 for mobile.

@ludoboludo ludoboludo self-assigned this May 10, 2023
Comment on lines 105 to 111
unless section.settings.mobile_layout == 'column' or section.blocks.size == 1
unless section.blocks.size == 3 and forloop.first and section.settings.desktop_layout == 'left'
unless section.blocks.size == 3 and forloop.last and section.settings.desktop_layout == 'right'
assign columns_amount_mobile = 2
endunless
endunless
endunless
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little confusing to read tbh.
If statements are easier to read than unless ones.

Suggested change
unless section.settings.mobile_layout == 'column' or section.blocks.size == 1
unless section.blocks.size == 3 and forloop.first and section.settings.desktop_layout == 'left'
unless section.blocks.size == 3 and forloop.last and section.settings.desktop_layout == 'right'
assign columns_amount_mobile = 2
endunless
endunless
endunless
if section.settings.mobile_layout == 'collage' and section.blocks.size > 1
unless section.blocks.size == 3 and forloop.first and section.settings.desktop_layout == 'left'
unless section.blocks.size == 3 and forloop.last and section.settings.desktop_layout == 'right'
assign columns_amount_mobile = 2
endunless
endunless
endif

As for the nesting of unless, not a big fan. We could simplify it by assigning some of those values to variables.

So having this:

assign is_large_block = false
if forloop.first and section.settings.desktop_layout == 'left'
  assign is_large_block = true
elsif forloop.last and section.settings.desktop_layout == 'right'
  assign is_large_block = true
endif 

to then do that:

assign columns_amount_mobile = 1
if section.settings.mobile_layout == 'collage' and section.blocks.size > 1
  unless section.blocks.size == 3 and is_large_block 
    assign columns_amount_mobile = 2
  endif
endif

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I adjusted the numbers assignment using the above:

  • Assign is_large_block first (as above)
  • Assign desktop columns if section.blocks.size > 1
  • Assign mobile columns (as above)

Comment on lines 122 to 141
{% liquid
unless section.blocks.size == 1
if forloop.first and section.settings.desktop_layout == 'left'
assign columns_amount_desktop = 1.5
elsif forloop.last and section.settings.desktop_layout == 'right'
assign columns_amount_desktop = 1.5
else
assign columns_amount_desktop = 3
endif
endunless

assign columns_amount_mobile = 1
unless section.settings.mobile_layout == 'column' or section.blocks.size == 1
unless section.blocks.size == 3 and forloop.first and section.settings.desktop_layout == 'left'
unless section.blocks.size == 3 and forloop.last and section.settings.desktop_layout == 'right'
assign columns_amount_mobile = 2
endunless
endunless
endunless
%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we need to repeat this twice 🤔 Since it just needs to be within the for loop. We could wrap it in an if statement if block.type == 'collection' or block.type == 'product' though that adds to the list of conditional already present.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for this one I wasn't sure what to do; having the block of code only once does seem more appealing.

it just needs to be within the for loop

My issue was "do we want to add an if check when there's already a case coming?
If we're fine with that I'm happy to move the logic before the case

@nate-young
Copy link

Just wanted to see if there was any updates here? It looks like it's still happening in versions 15.0.2.

Additionally, the same issue appears to effect article cards.

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

Successfully merging this pull request may close these issues.

5 participants