Change thumbnail media fit to "fill" by default #2044

kmeleta · 2022-10-18T00:57:34Z

PR Summary:

Allows product thumbnail images to fill the available space.

Why are these changes introduced?

Fixes #2047
Refs #2016

As part of upcoming product media sizing updates, it was decided to change the media fit method of the product media thumbnails from a "contain" approach (scale down to fit) to a "fill" or "cover" approach (scale up/crop to fill). Though associated with the other product media sizing work, the functionality is entirely separate as implemented and can be tested in isolation.

What approach did you take?

Previously the thumbnails were manually creating a object-fit: contain type of effect using thumbnail--wide/narrow classes. I removed those classes and the associated styles and applied a object-fit: cover. I previously implemented a more complex approach also using the wide/narrow classes, but found it unnecessary.

I also replaced the existing img elements used for thumbnails with image_tag and adjusted some srcset/sizes. There were some existing inconsistencies in the two instances of the srcsets/sizes anyway, which I've attempted to correct. With the new cover fit approach, the images can technically render larger than the thumbnail containers so special attention should be given to ensure adequate image sources are loading.

Visual impact on existing themes

Current behavior

New behavior

Testing steps/scenarios

Main product section with Desktop layout set to Thumbnail carousel or Thumbnails
Main product section with Mobile layout set to Show thumbnails
Ensure non-square images fill the square thumbnails as expected
Ensure images are centered within the thumbnail
Ensure image sources loaded are of adequate size
Ensure there are no regressions to any aspect of the thumbnail UI

"A/ Image sizing test – Different aspect ratios" and "Puppy" are good test products but try a variety.

Demo links

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

duygukalaycioglu

Looking good! 🎉

assets/section-main-product.css

ludoboludo · 2022-10-26T15:48:27Z

snippets/product-media-gallery.liquid

@@ -41,6 +41,8 @@
  if is_duplicate
    assign id_append = '-duplicate'
  endif
+
+  assign thumbnail_media_fit = 'cover'


Do we really care about a "fit" option for thumbnails ? I don't expect anyone to really rely on a thumbnail image to view all the details 🤷
I feel like we could remove the original approach and have the cover one by default. 🤔

I don't expect anyone to really rely on a thumbnail image to view all the details

That's the expectation. But depending on merchant feedback, I also wouldn't be surprised if at some point we end up offering a media fit setting for it. Though the variable here seems a bit unnecessary to me now, for sure. How would you feel about keeping the media-fit-{{thumbnail_media_fit}} class on the thumbnail elements but have it hard coded to media-fit-cover

That would mean we might need to refactor in the future if we don't have feedback requesting us to re add the fit option right ?
I'm not a huge fan of adding follow up work but if we suspect there is a good chance for that to happen, why not 🤔 I could see how some zoomed in Portrait images could be cropped in a way that the thumbnail is too zoomed in.

And when I say refactor I mean if it's the default and only behaviour we then don't need to create new classes but just adjust the old ones.

I outline my reasons for preserving the code in the decision log.

But leaving the current/contain stuff would mean adding a setting is as easy as adding media-fit-{{settings.thumb_fit}}. I believe this is also why I added this line you correctly question .thumbnail--narrow:not(.media-fit-cover) img. It's just leaving the door open for this without actually using the word "contain" anywhere (since it doesn't exist as an option currently).

I could see unused code being a little confusing to a dev, but it's also only keeping like 5-6 lines of css around 🤷 I also feel having the media-fit-cover class helps clarify what the odd absolute positioning styles are actually doing. So I'm torn.. maybe it's not worth having to defend in the future.

Heh, so I decided I couldn't defend my decision and went in to clean stuff up. Also decided to try again to get a simple object-fit to work while I was at it. Seems to work fine if I remove all the styles that manually attempted to create the original contain effect. So I basically just removed a bunch of new and preexisting stuff 0532ff8 😂 Not sure why we weren't already using object-fit for these.

ludoboludo · 2022-10-26T15:51:11Z

snippets/product-media-gallery.liquid

+          (min-width: {{ settings.page_width }}px) calc(({{ settings.page_width | minus: 100 | times: media_width | round }} - 4rem) / 4),
+          (min-width: 990px) calc(({{ media_width | times: 100 }}vw - 4rem) / 4),
+          (min-width: 750px) calc((100vw - 15rem) / 8),
+          calc((100vw - 8rem) / 3)


Any reason why we went from calc((100vw - 14rem) / 3) to calc((100vw - 8rem) / 3)

I will say the crux of this PR is the image size stuff and there could be cases I'm missing, but like I mentioned about the srcset, with the cover fit in place, I've kind of been rounding up to avoid having to create an unnecessarily complex sets of conditionals to account for possible setting/screen combination.

For this one specifically, I know there was a specific mobile case where a landscape image was rendering very blurry with the original size. I believe I sort of combined the existing mobile size logic here with the "other" (un-updated) instance in the loop below which was originally calc((100vw - 8rem) / 5). The 5 seemed like a mistake because mobile only shows 3 thumbs, but I otherwise felt this had a better result than calc((100vw - 14rem) / 3)

I don't know, maybe something like 11rem would be better than 8 but I'm not sure where 14 came from regardless.

Gotcha 👍 thanks for the context.

snippets/product-media-gallery.liquid

ludoboludo · 2022-10-26T15:59:44Z

snippets/product-media-gallery.liquid

+                {{ media.preview_image | image_url: width: 416 | image_tag:
+                  loading: 'lazy',
+                  sizes: sizes,
+                  widths: '54, 74, 104, 162, 208, 324, 416',


Any specific reason why we change the original widths selected ? When I test quickly it seems like the wider the thumbnail is going to get is about 155px. So we shouldn't need to output anything over 300px 🤔

Well I mostly changed them to match the featured media thumbnail above this loop. It somewhat seemed like that instance was updated at some point and this one was forgotten about (this is why DRY is a valid consideration). I can't think of an instance where the first thumbnail should ever have different image size requirements than the subsequent ones. But maybe I'm missing something.

In any case though, with the cover media fit, images could actually render larger than the container and would appear blurry if a larger enough src isn't available.

ludoboludo

It's looking good.

It does crop a bit the images that are long portrait and where the focus isn't the middle of the image but I think it's a pretty rare case 👍
video

kmeleta · 2022-10-27T17:59:59Z

It does crop a bit the images that are long portrait and where the focus isn't the middle of the image but I think it's a pretty rare case 👍 video

Hmm am I crazy? That one is pretty long so it's kind of hard to tell but that looks pretty centered to me @ludoboludo

ludoboludo · 2022-10-27T20:32:59Z

Yeah yeah, what I meant is that if you're aiming to have focus on the face in this case, you'd need to specific it in the CSS. But it's an edge case that shouldn't be too problematic I think

eugenekasimov

Works as expected!🚀

* Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach

commit dc81806 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Tue Nov 22 13:32:46 2022 -0700 Add media sizing settings to featured product (Shopify#2114) commit 9875aea Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Tue Nov 22 13:17:55 2022 -0700 Adjust constrain media values for quick add modal (Shopify#2113) * Adjust constrain media values for quickadd * Adjust offset logic * Minor formatting fix commit 2eb8bf6 Author: Ludo <ludo.segura@shopify.com> Date: Tue Nov 22 11:50:50 2022 -0500 Add fixed sizes for some images to prevent errors (Shopify#2087) * remove the mention of sizes where unnecessary * remove another instance * add fixed size to the images in structured data * adjust size to 1920 * edit logo size in structured data * remove conditional info * change value to match other logo size values commit 2f9b9f3 Author: Ludo <ludo.segura@shopify.com> Date: Mon Nov 21 15:44:24 2022 -0500 Remove alt attribute in image_tag filter where unnecessary (Shopify#2117) * Add fallback to alt attribute where image_tag filter is used * remove unnecessary use of alt attribute in image_tag filter commit 251e5d9 Author: Ludo <ludo.segura@shopify.com> Date: Mon Nov 21 12:18:44 2022 -0500 Move share button into a snippet (Shopify#2123) * Move share button into a snippet * change the property name to context and add details in snippet * address review comments * edit props names and description * Address reviewers comment. Remove unused prop and edit existing ID * move script at the top commit 539af27 Author: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com> Date: Mon Nov 21 09:01:58 2022 -0800 Prettier liquid files snippets. (Shopify#2115) commit 503fdf8 Author: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com> Date: Thu Nov 17 08:51:21 2022 -0800 Remove noscript css import for product recommendations (Shopify#2101) commit b2b01fd Author: Eugene Kasimov <105315663+eugenekasimov@users.noreply.github.com> Date: Thu Nov 17 08:50:33 2022 -0800 Change variables names (Shopify#2055) commit cf05d0d Author: Francisca Calabria Rubio <13103124+FCalabria@users.noreply.github.com> Date: Thu Nov 17 16:50:55 2022 +0100 Select text input content on trapFocus (Shopify#2106) * Select text input content on trapFocus * PR fix commit b86d1f3 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Thu Nov 17 08:50:43 2022 -0700 Change thumbnail media fit to "fill" by default (Shopify#2044) * Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach commit 8ab8e61 Author: Ludo <ludo.segura@shopify.com> Date: Thu Nov 17 10:50:28 2022 -0500 Tweak the image_url size to be the max value where necessary (Shopify#2110) * Tweak the image_url size to be the max value where necessary * fix sizing in the header. commit c337301 Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Nov 17 10:49:52 2022 -0500 Update 1 translation file (Shopify#2130) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit bc8f433 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Thu Nov 17 08:49:10 2022 -0700 Add constrain media and media fit settings to product page (Shopify#2052) * Add constrain height setting * Add constrain scaling functionality * Add media fit setting and functionality * Quick add override and typo fixes * Update setting language and code cosmetics * Update 8 translation files * Update 5 translation files * Update 3 translation files * Update 3 translation files * Update 1 translation file * Add comment Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com>

* Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach

kmeleta marked this pull request as ready for review October 18, 2022 16:24

kmeleta requested a review from duygukalaycioglu October 18, 2022 16:25

kmeleta changed the title ~~Change thumbnail media fit to cover by default~~ Change thumbnail media fit to fill by default Oct 18, 2022

duygukalaycioglu changed the title ~~Change thumbnail media fit to fill by default~~ Change thumbnail media fit to "fill" by default Oct 18, 2022

duygukalaycioglu previously approved these changes Oct 18, 2022

View reviewed changes

ludoboludo reviewed Oct 26, 2022

View reviewed changes

eugenekasimov self-requested a review October 26, 2022 18:56

kmeleta added 2 commits October 26, 2022 16:58

Change thumbnail media fit to cover by default

f7683a8

Simplify and cleanup thumbnail fit approach

0532ff8

kmeleta dismissed duygukalaycioglu’s stale review via 0532ff8 October 26, 2022 23:10

kmeleta force-pushed the thumbnail-fit-cover branch from 194fee0 to 0532ff8 Compare October 26, 2022 23:10

ludoboludo approved these changes Oct 27, 2022

View reviewed changes

eugenekasimov approved these changes Oct 28, 2022

View reviewed changes

duygukalaycioglu approved these changes Oct 30, 2022

View reviewed changes

kmeleta merged commit b86d1f3 into main Nov 17, 2022

kmeleta deleted the thumbnail-fit-cover branch November 17, 2022 15:50

chavdar-ang pushed a commit to chavdar-ang/lermitage that referenced this pull request Nov 20, 2022

Change thumbnail media fit to "fill" by default (Shopify#2044)

1dd4d97

* Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach

phapsidesGT pushed a commit to Gravytrain-UK/gt-shopify-dawn-theme that referenced this pull request Sep 3, 2024

Change thumbnail media fit to "fill" by default (Shopify#2044)

731f18c

* Change thumbnail media fit to cover by default * Simplify and cleanup thumbnail fit approach

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change thumbnail media fit to "fill" by default #2044

Change thumbnail media fit to "fill" by default #2044

kmeleta commented Oct 18, 2022 •

edited

Loading

duygukalaycioglu left a comment

ludoboludo Oct 26, 2022

kmeleta Oct 26, 2022

ludoboludo Oct 26, 2022

kmeleta Oct 26, 2022 •

edited

Loading

kmeleta Oct 26, 2022

ludoboludo Oct 26, 2022

kmeleta Oct 26, 2022

ludoboludo Oct 26, 2022

ludoboludo Oct 26, 2022

kmeleta Oct 26, 2022 •

edited

Loading

ludoboludo left a comment

kmeleta commented Oct 27, 2022

ludoboludo commented Oct 27, 2022

eugenekasimov left a comment

Change thumbnail media fit to "fill" by default #2044

Change thumbnail media fit to "fill" by default #2044

Conversation

kmeleta commented Oct 18, 2022 • edited Loading

PR Summary:

Why are these changes introduced?

What approach did you take?

Visual impact on existing themes

Testing steps/scenarios

Demo links

Checklist

duygukalaycioglu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmeleta Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

kmeleta commented Oct 27, 2022

ludoboludo commented Oct 27, 2022

eugenekasimov left a comment

Choose a reason for hiding this comment

kmeleta commented Oct 18, 2022 •

edited

Loading

kmeleta Oct 26, 2022 •

edited

Loading

kmeleta Oct 26, 2022 •

edited

Loading