-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
c25500b
bf9813b
262e4e3
8db58fd
ea3fc19
6b2b1e8
29ed08b
1ac370a
691a640
2111487
7e50a15
015ef60
ddf1180
a9914ad
72c13c5
aa247b1
9cbd431
a1dfcd7
3bb16d3
2db55d5
3b109c6
7ea1e11
e1ada57
a70d1e6
f4835a1
63ef7e3
0936ace
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,7 +4,8 @@ | |
Accepts: | ||
- card_collection: {Object} Collection Liquid object | ||
- media_aspect_ratio: {String} Size of the product image card. Values are "square" and "portrait". Default is "square" (optional) | ||
- columns: {Number} | ||
- columns_desktop: {Number} Number of columns on desktop. Default: 3 (optional) | ||
- columns_mobile: {Number} Number of columns on mobile. Default: 2 (optional) | ||
- extend_height: {Boolean} Card height extends to available container space. Default: false (optional) | ||
- wrapper_class: {String} Wrapper class for card (optional) | ||
|
||
|
@@ -28,6 +29,8 @@ | |
assign card_color_scheme = settings.collection_card_color_scheme | ||
assign card_style = settings.collection_card_style | ||
endif | ||
assign desktop = columns_desktop | at_least: 1 | ||
assign mobile = columns_mobile | at_least: 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
-%} | ||
|
||
<div class="card-wrapper animate-arrow {% if wrapper_class and wrapper_class != 'none' %}{{ wrapper_class }}{% else %}collection-card-wrapper{% endif %}"> | ||
|
@@ -62,8 +65,8 @@ | |
" | ||
src="{{ card_collection.featured_image | image_url: width: 1500 }}" | ||
sizes=" | ||
(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 100 | divided_by: columns }}px, | ||
(min-width: 750px) {% if columns > 1 %}calc((100vw - 10rem) / 2){% else %}calc(100vw - 10rem){% endif %}, | ||
(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 100 | divided_by: desktop }}px, | ||
(min-width: 750px) {% if mobile > 1 %}calc((100vw - 10rem) / 2){% else %}calc(100vw - 10rem){% endif %}, | ||
calc(100vw - 3rem) | ||
" | ||
alt="" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,8 @@ | |
- section_id: {String} The ID of the section that contains this card. | ||
- horizontal_class: {Boolean} Add a card--horizontal class if set to true. Default: false (optional) | ||
- horizontal_quick_add: {Boolean} Changes the quick add button styles when set to true. Default: false (optional) | ||
- columns_desktop: {Number} Number of columns on desktop. Default: 4 (optional) | ||
- columns_mobile: {Number} Number of columns on mobile. Default: 2 (optional) | ||
|
||
Usage: | ||
{% render 'card-product', show_vendor: section.settings.show_vendor %} | ||
|
@@ -31,6 +33,8 @@ | |
if ratio == 0 or ratio == null | ||
assign ratio = 1 | ||
endif | ||
assign desktop = columns_desktop | at_least: 1 | ||
assign mobile = columns_mobile | at_least: 1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
-%} | ||
<div class="card-wrapper product-card-wrapper underline-links-hover"> | ||
<div | ||
|
@@ -64,7 +68,7 @@ | |
{{ card_product.featured_media | image_url }} {{ card_product.featured_media.width }}w | ||
" | ||
src="{{ card_product.featured_media | image_url: width: 533 }}" | ||
sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 130 | divided_by: 4 }}px, (min-width: 990px) calc((100vw - 130px) / 4), (min-width: 750px) calc((100vw - 120px) / 3), calc((100vw - 35px) / 2)" | ||
sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 130 | divided_by: desktop }}px, (min-width: 990px) calc((100vw - 130px) / {{ desktop }}), (min-width: 750px) calc((100vw - 120px) / {{ mobile }}), calc((100vw - 35px) / {{ mobile }})" | ||
alt="{{ card_product.featured_media.alt | escape }}" | ||
class="motion-reduce" | ||
{% unless lazy_load == false %} | ||
|
@@ -87,7 +91,7 @@ | |
{{ card_product.media[1] | image_url }} {{ card_product.media[1].width }}w | ||
" | ||
src="{{ card_product.media[1] | image_url: width: 533 }}" | ||
sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 130 | divided_by: 4 }}px, (min-width: 990px) calc((100vw - 130px) / 4), (min-width: 750px) calc((100vw - 120px) / 3), calc((100vw - 35px) / 2)" | ||
sizes="(min-width: {{ settings.page_width }}px) {{ settings.page_width | minus: 130 | divided_by: desktop }}px, (min-width: 990px) calc((100vw - 130px) / {{ desktop }}), (min-width: 750px) calc((100vw - 120px) / {{ mobile }}), calc((100vw - 35px) / {{ mobile }})" | ||
alt="" | ||
class="motion-reduce" | ||
loading="lazy" | ||
|
There was a problem hiding this comment.
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 assignedat_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: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 potentially140px
when it's on a product with no image. And118px
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 and5
for mobile 👍 This way we don't go into logic that's a bit hard to understand.What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
That was also my concern 😅
Haven't tried any of this yet; was just a draft starting from what sounded logical.
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)
There was a problem hiding this comment.
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:
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 withdesktop_layout = 'right'
and this fixed it.There was a problem hiding this comment.
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).