Improve image sizes in the multicolumn section #2349

eugenekasimov · 2023-03-01T05:46:41Z

PR Summary:

This PR improves the way images are loaded in the multicolumn section.

Why are these changes introduced?

What approach did you take?

I rewrote the logic of how we create our srcset. Now it accounts for the page width, desktop/tablet/mobile layouts, number of columns, the size of the image(full, half or third). The logic is based on the most common pixel density 2 and I also added options for pixel density 1.

All calculations are based on the maximum possible image sizes for each media query and don't account for paddings/margin etc.

Other considerations

Maybe we could add more options for pixel density 3 since many latest mobile devices have it.
We could also add more breakpoints to cover more scenarios in terms of screen size.

Decision log

Visual impact on existing themes

There is no visual impact.

Testing steps/scenarios

Set the multicolumn section with images.
Test different settings such a different image with and/or different number of columns and using the devtool check the intrinsic size and make sure it calculates right.

Demo links

Store
Editor

Checklist

Added PR summary for release notes
Requested review from UX (Only for changes that are affecting the experience or perceivable visual details)
Created a ticket for the help.shopify.com documentation team about updates to theme settings. (Internal-only task)
Followed theme code principles
Linted with Theme Check
Tested on mobile
Tested on multiple browsers
Tested for accessibility

stufreen

Way better than before! It looks like the previous version was based on the total number of blocks, which just seems wrong. There must have been an update but the sizes property wasn't updated.

sections/multicolumn.liquid

stufreen · 2023-03-07T20:06:48Z

sections/multicolumn.liquid

+                      (min-width: 750px) {{ 989 | times: imageWidth | divided_by: numberOfColumnsMobile | round: 0 }}px,
+                      (min-width: 375px) {{ 749 | times: imageWidth | divided_by: numberOfColumnsMobile | round: 0 }}px,
+                      {{ 375 | times: imageWidth | divided_by: numberOfColumnsMobile | round: 0 }}px


The only issue I see with this is that it will always jump to the next size... E.g. if the screen width is 375 it will use an image that is 749w (assuming imageWidth of 1).

I think you can replace these three lines with:

calc(100vw * {{ imageWidth | divided_by: numberOfColumnsMobile }})

I think we can be more flexible/accurate here without much more complexity if we apply the same sort of math to css calculations replacing fixed values like 989 with 100vw. This will essentially create a dynamic value that also factors in the viewport width, rather than just the max breakpoint width.

Also I think we often define 3 breakpoints and a non-specific mobile size that is essentially a max-width: 749px. I think if you make the change above you'll find you don't need the min-width 375 breakpoint

min-width: {{ settings.page_width}} (image sizes won't vary above this breakpoint and can use a fixed sized like you have currently)

min-width: 990px (image sizes vary with viewport between 990 and page_width)

min-width: 750px (image sizes vary with viewport between 750 and 990)

I decided to go with what Stu suggested 👆.

sections/multicolumn.liquid

kmeleta · 2023-03-06T16:59:53Z

sections/multicolumn.liquid

-                      (min-width: 990px) {% if section.blocks.size <= 2 %}710px{% else %}550px{% endif %}, (min-width:
-                      750px) {% if section.blocks.size == 1 %}710px{% else %}550px{% endif %}, calc(100vw - 30px)
+                    {%- liquid
+                      assign numberOfColumns = section.settings.columns_desktop


Obvi super nitpicky but we don't usually use camel case for liquid var names. So I'd change these to number_of_columns Though feel free to shorten these particular ones a bit too by abbreviating "number" and excluding the "of". num_columns and num_columns_mobile is still perfectly clear imo.

kmeleta · 2023-03-07T21:28:16Z

sections/multicolumn.liquid

+                      (min-width: 750px) {{ 989 | times: imageWidth | divided_by: numberOfColumnsMobile | round: 0 }}px,
+                      (min-width: 375px) {{ 749 | times: imageWidth | divided_by: numberOfColumnsMobile | round: 0 }}px,
+                      {{ 375 | times: imageWidth | divided_by: numberOfColumnsMobile | round: 0 }}px


I think we can be more flexible/accurate here without much more complexity if we apply the same sort of math to css calculations replacing fixed values like 989 with 100vw. This will essentially create a dynamic value that also factors in the viewport width, rather than just the max breakpoint width.

Also I think we often define 3 breakpoints and a non-specific mobile size that is essentially a max-width: 749px. I think if you make the change above you'll find you don't need the min-width 375 breakpoint

min-width: {{ settings.page_width}} (image sizes won't vary above this breakpoint and can use a fixed sized like you have currently)

min-width: 990px (image sizes vary with viewport between 990 and page_width)

min-width: 750px (image sizes vary with viewport between 750 and 990)

sections/multicolumn.liquid

kmeleta · 2023-03-08T21:18:49Z

sections/multicolumn.liquid

+                      (min-width: 990px) {{ settings.page_width | times: image_width | divided_by: number_of_columns | round: 0 }}px,
+                      calc(100vw * {{ image_width }} / {{ number_of_columns_mobile }})


The mobile logic is improved, which is important because there's a pretty big range images can render at between 0 and 750px. But the 990px breakpoint is still a static number and will usually render too large of an image, with the worst case scenario being a viewport width closer to 990 with a page_width set closer to 1600.

I mentioned the strategy we use in other sections here but something like this is more or less what I would expect.

(min-width: {{ settings.page_width }}px) {{ settings.page_width | divided_by: num_columns }}px, (min-width: 990px) calc((100vw - desktop_padding?) / {{ num_columns }} * {{ image_width }}), (min-width: 750px) calc((100vw - mobile_padding?) / {{ num_columns_mobile }} * {{ image_width }}), calc(100vw / {{ num_columns_mobile }} * image_width)

If we're not going to factor any padding values into these calculations, we can probably skip the 750px breakpoint, but I think we should definitely improve the larger breakpoints somehow at least.

Oh, what you are trying to say is that when the page-width is set to 1600px and a view port of the screen is smaller we are not really efficient. Is that right?
That's a good catch @kmeleta . I'm going to fix it but still skip the 750px. Does it sound good to you?

Bottomline is if we can get good results without accounting for paddings, then we don't need to consider them and enjoy the simpler logic. The desktop and tablet page paddings are 50px (100px total), the gaps between columns can be between 4px and 40px (40px * (num_columns - 1) or 200px worst case total). So in theory, under some conditions there could be a calculated discrepancy of up to ~300px.

Here's an example factoring in more grid spacing. Pretty inaccurate result, but of ourse with 6 columns the general image sizes are at least smaller. What's your feeling?

I reviewed everything again and decided to add paddings. I think it's worth it. Now I feel we are quite precise in the calculations even though I had to hardcode some of the padding values.

stufreen

Working really well from my testing - just a style comment

sections/multicolumn.liquid

kmeleta

Much better results now 🚀 I'll also document the hardcoded widths as standard approach now.

melissaperreault

🎉 I tried to see if any of this change would be perceivable with a before and after comparison. (Video reference) Looks good!

eugenekasimov · 2023-03-10T17:57:09Z

Thank you all for the great feedback and giving me a good direction for this PR. 🚀

* shopify/main: Improve image sizes in the multicolumn section (Shopify#2349) Fix the Page section's width. (Shopify#2364) Update 12 translation files (Shopify#2366) Removing "my" from cart popup notification (Shopify#2353) [Cart.js] Fix fetch url so it's not hard coded (Shopify#2357) (Shopify#2365) Update 1 translation file (Shopify#2352) Default Follow on Shop to on [Header] Add localization selectors (Shopify#2258) Remove async CSS pattern where it may introduce layout shifts (Shopify#2270) Change rich text section heading to be of type inline_richtext, also moved rte styling into base.css (Shopify#2326) Add drawer menu desktop (Shopify#2195) Make header image preload and proper width (Shopify#2307)

* Update image settings for multicolumn. * Add more breakpoints. * Change naming. Add arbitrary number for widths. Improve sizes.: * Add more numbers to widths. * Add paddings for calculation * Add variables

eugenekasimov added 2 commits February 28, 2023 10:24

Update image settings for multicolumn.

0fa7282

Add more breakpoints.

0a11554

eugenekasimov requested review from kmeleta and stufreen March 1, 2023 18:15

melissaperreault self-requested a review March 3, 2023 18:45

kmeleta self-assigned this Mar 6, 2023

stufreen suggested changes Mar 7, 2023

View reviewed changes

kmeleta reviewed Mar 7, 2023

View reviewed changes

Change naming. Add arbitrary number for widths. Improve sizes.:

5a82973

eugenekasimov force-pushed the fix-image-sizes-multicolumn branch from 9ab1706 to 5a82973 Compare March 8, 2023 17:02

eugenekasimov requested review from stufreen and kmeleta March 8, 2023 17:02

kmeleta reviewed Mar 8, 2023

View reviewed changes

sections/multicolumn.liquid Outdated Show resolved Hide resolved

stufreen previously approved these changes Mar 8, 2023

View reviewed changes

Add more numbers to widths.

708cfbb

eugenekasimov dismissed stufreen’s stale review via 708cfbb March 8, 2023 19:14

eugenekasimov requested review from stufreen and kmeleta March 8, 2023 19:18

kmeleta reviewed Mar 8, 2023

View reviewed changes

Add paddings for calculation

6278df7

eugenekasimov requested a review from kmeleta March 9, 2023 18:34

stufreen previously approved these changes Mar 9, 2023

View reviewed changes

sections/multicolumn.liquid Outdated Show resolved Hide resolved

Add variables

2b44b74

eugenekasimov dismissed stufreen’s stale review via 2b44b74 March 9, 2023 22:07

eugenekasimov requested a review from stufreen March 9, 2023 22:08

stufreen approved these changes Mar 9, 2023

View reviewed changes

kmeleta approved these changes Mar 10, 2023

View reviewed changes

melissaperreault approved these changes Mar 10, 2023

View reviewed changes

eugenekasimov merged commit d2580da into main Mar 10, 2023

eugenekasimov deleted the fix-image-sizes-multicolumn branch March 10, 2023 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve image sizes in the multicolumn section #2349

Improve image sizes in the multicolumn section #2349

eugenekasimov commented Mar 1, 2023 •

edited

Loading

stufreen left a comment

stufreen Mar 7, 2023

kmeleta Mar 7, 2023 •

edited

Loading

eugenekasimov Mar 8, 2023

kmeleta Mar 6, 2023

kmeleta Mar 7, 2023 •

edited

Loading

kmeleta Mar 8, 2023 •

edited

Loading

eugenekasimov Mar 8, 2023 •

edited

Loading

kmeleta Mar 8, 2023

eugenekasimov Mar 9, 2023 •

edited

Loading

stufreen left a comment

kmeleta left a comment

melissaperreault left a comment

eugenekasimov commented Mar 10, 2023

		(min-width: 990px) {{ settings.page_width \| times: image_width \| divided_by: number_of_columns \| round: 0 }}px,
		calc(100vw * {{ image_width }} / {{ number_of_columns_mobile }})

Improve image sizes in the multicolumn section #2349

Improve image sizes in the multicolumn section #2349

Conversation

eugenekasimov commented Mar 1, 2023 • edited Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Other considerations

Decision log

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

stufreen left a comment

Choose a reason for hiding this comment

stufreen Mar 7, 2023

Choose a reason for hiding this comment

kmeleta Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

eugenekasimov Mar 8, 2023

Choose a reason for hiding this comment

kmeleta Mar 6, 2023

Choose a reason for hiding this comment

kmeleta Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

kmeleta Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

eugenekasimov Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

kmeleta Mar 8, 2023

Choose a reason for hiding this comment

eugenekasimov Mar 9, 2023 • edited Loading

Choose a reason for hiding this comment

stufreen left a comment

Choose a reason for hiding this comment

kmeleta left a comment

Choose a reason for hiding this comment

melissaperreault left a comment

Choose a reason for hiding this comment

eugenekasimov commented Mar 10, 2023

eugenekasimov commented Mar 1, 2023 •

edited

Loading

kmeleta Mar 7, 2023 •

edited

Loading

kmeleta Mar 7, 2023 •

edited

Loading

kmeleta Mar 8, 2023 •

edited

Loading

eugenekasimov Mar 8, 2023 •

edited

Loading

eugenekasimov Mar 9, 2023 •

edited

Loading