Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?
Fix blurry product/collection grid images #2277
Changes from 1 commit
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
There are no files selected for viewing
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).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.
Same here for the naming. Having something a bit more descriptive so it's easier to read would be great.
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.
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