Product Media Layout updates #937

LucasLacerdaUX · 2021-11-23T06:28:00Z

Why are these changes introduced?
Adds three new settings to product page:

Large screens layout
Media size
Mobile thumbnails

Closes #828.

What approach did you take?

Created a new MediaGallery component to manage the GalleryViewer. This component manages two sliders (gallery and thumbnails) and keeps them in sync. It also serves as an interface for other components, like VariantSwitch. Instead of changing the DOM directly, we now make this request via a setActiveMedia(media, prepend) function.
Thumbnails are implemented using a <slider-component>. This is different to the dots/numbers in the Slideshow component that @ludoboludo implemented on Slideshow section #840, because both thumbnails and viewer can be controlled separately. The same HTML is used for both Mobile and Desktop.

Setting 1: Large screens layout

Stacked: if sticky column setting is enabled, info column is sticky.
Thumbnails: if sticky column setting is enabled, media column is sticky. Will limit the height of media.

Setting 2: Media size

Small: 45% media / 55% content. 4 thumbnails per row. 20x20 thumbnail badges. 500px maximum height if Thumbnails layout is set.
Medium: 55% media / 45% content. 5 thumbnails per row. 30x30 thumbnail badges. 600px maximum height if Thumbnails layout is set.
Large: 65% media / 35% content. 6 thumbnails per row. 30x30 thumbnail badges. 600px maximum height if Thumbnails layout is set.

Setting 3: Mobile thumbnails

Hide will use < 1/3 > format for slider.
Show can be enabled independently from the Large screens layout setting. Uses the same Thumbnails component, but inside a slider. Pressing the next/prev button will scroll 3 slides at a time. If user scrolls the main gallery slider, the thumbnail slider will be kept in sync.

Accessibility:

Thumbnails (button) have a aria-controls="GalleryViewer-{{ section.id }}" and are labelled (via aria-label) depending on the media type:
- Images: Load image {{ index }} in gallery view
- 3D Models: Load 3D Model {{ index }} in gallery view
- Videos: Play video {{ index }} in gallery view
If set to Thumbnails mode, the gallery viewer will have a <h2>Gallery Viewer</h2> at the top.
When you click on a Thumbnail:
- If it's a video or 3D Model, it will autoplay.
- If it's an image, a role="status" div will receive Image {{ index }} is now available in gallery view once the <img> is loaded for the first time.

Demo links

Store
Editor

Checklist

locales/en.default.json

ludoboludo

Looks really good. Found the code nice to read as well 👌

Tested quite a few scenarios and left a few notes on what I noticed.

assets/global.js

assets/section-main-product.css

sections/main-product.liquid

assets/global.js

ludoboludo · 2021-11-25T21:21:24Z

assets/global.js

  onButtonClick(event) {
    event.preventDefault();
-    const slideScrollPosition = event.currentTarget.name === 'next' ? this.slider.scrollLeft + this.sliderLastItem.clientWidth : this.slider.scrollLeft - this.sliderLastItem.clientWidth;
+    const step = event.currentTarget.dataset.step || 1;
+    const slideScrollPosition = event.currentTarget.name === 'next' ? this.slider.scrollLeft + (step * this.sliderLastItem.clientWidth) : this.slider.scrollLeft - (step * this.sliderLastItem.clientWidth);


There is a bit of a delay happening sometimes between the thumbnail sliding to the currently active image: video

I have no idea how to fix this without a big refactor on the slider component.

I don't think it should be a blocker, though. Would it be okay to solve this on follow up PRs?

Yeah! not a blocker at all 👍

assets/global.js

svinkle · 2021-11-26T18:34:49Z

Initial Desktop first-pass observations:

On first click of a video, focus shifts to the video or related iframe element. Subsequent clicks do not shift focus. Focus should remain on the thumbnail button.
Each media type should have its own index. This helps to provide accurate control accessible naming.

Ex., tabbing through thumbnail controls with mixed media (2 images, then a video, then another image, then a 3D model) would ideally announce:

🗣 "Load image 1 in gallery view Tab Load image 2 in gallery view Tab Play video 1 in gallery view Tab Load image 3 in gallery view Tab Load 3D Model 1 in gallery view"
The gallery viewer markup in this view needs some adjustment. It doesn't make sense to reuse the list structure for the gallery view since there will ever only be 1 list item presented at a time.

When the conditions of the Thumbnail layout and desktop viewport are true, inside the slider-component element, remove semantic meaning by applying role="presentation" to the ul and each li.
The heading I suggested in Figma throws off the heading order. Let's remove this but instead, create a landmark for the gallery viewer.

Add aria-label="Gallery viewer" and role="region" to the media-gallery component.
I see the role="status" in the DOM but I don't see any updates being pushed to this element. I believe there should be something pushed for when an image has loaded?

tyleralsbury

After a quick pass on the code, this is looking really great. I only have one comment so far, plus the vid I sent you on Slack. Great catches by Ludo as well. 👍

tyleralsbury · 2021-11-26T18:54:25Z

snippets/product-thumbnail.liquid

+        srcset="{% if media.preview_image.width >= 288 %}{{ media.preview_image | img_url: '288x' }} 288w,{% endif %}
+          {% if media.preview_image.width >= 576 %}{{ media.preview_image | img_url: '576x' }} 576w,{% endif %}
+          {% if media.preview_image.width >= 750 %}{{ media.preview_image | img_url: '750x' }} 750w,{% endif %}
+          {% if media.preview_image.width >= 1100 %}{{ media.preview_image | img_url: '1100x' }} 1100w,{% endif %}
+          {% if media.preview_image.width >= 1500 %}{{ media.preview_image | img_url: '1500x' }} 1500w,{% endif %}
+          {{ media.preview_image | img_url: 'master' }} {{ media.preview_image.width }}w"
+        src="{{ media | img_url: '1500x' }}"


There will need to be a multiplier on the values in the gallery for optimal desktop image sizes dependent on the width of the gallery. Ping me if you want details, but for example... if the original width of the container was 50% and an image size for that is 1000, and the setting is now 60%, the image needs to be 1200 when that setting is selected.

I think it will take some assigns, unfortuantely.

svinkle · 2021-11-29T16:11:56Z

After adding alt text to the thumbnails on this page, I realize each thumbnail control has very little context to what is actually being loaded into the viewer.

I'd like to propose including hint text onto each button. Add the aria-describedby attribute on each, referencing the id of its image thumbnail.

<button
  class="thumbnail thumbnail--narrow"
  aria-label="{btn.label}"
  aria-controls="GalleryViewer-template--15085127434262__main"
  aria-describedby="thumbnail-{i}"
>
  <img
    id="thumbnail-{i}"
    src="{img.src}}"
    alt="{img.alt}"
    height="200"
    width="200"
    …
  />
</button>

With this screen readers will announce:

🗣 "Load image {i} into gallery viewer, button, {img.alt}"

svinkle · 2021-11-29T16:26:59Z

In addition to the desktop review, here's some notes on the mobile experience:

When swiping through ul.product__media-list, each item was loaded into the gallery viewer. Ideally items would be hidden until a thumbnail control has been activated revealing the media (similar to how desktop behaves.)
Each gallery viewer button.product__media-toggle includes a generic "open media {i}" label. Ideally these would be updated to reference the media type and its unique type index, as on the desktop view. (Load image, play video, load 3D model, etc.)

svinkle · 2021-11-30T16:43:20Z

Product images are not visible in Windows Hight Contrast mode in Firefox. Adjust the CSS of the button to use the :after pseudo element. Review the product card CSS as an example.

duygukalaycioglu

Decrease the spacing below the image and the thumbnails on mobile.

When the thumbnails are hidden, decrease the spacing above the product title on mobile.

Thanks!

katycobb · 2021-12-07T21:41:58Z

Hi @LucasLacerdaUX I've been working with @duygukalaycioglu to make some content updates. They're reflected in the Figma file + listed below. Let me know if you need more info, and D let me know if there's anything I've missed!

Checkbox copy: Enable sticky content on desktop
Large screens layout --> Desktop layout
Add help text below Desktop layout: Thumbnails is the default mobile layout.
Media size --> Desktop media size
Add help text below Desktop media size: Media is automatically optimized for mobile.

LucasLacerdaUX · 2021-12-07T22:08:16Z

@katycobb @duygukalaycioglu I have a question about:

Thumbnails is the default mobile layout.

Does that mean we are changing Mobile thumbnails default from Hide to Show? Right now, the default is Hide.

Even if we are updating that setting's default, the closest option to the default mobile experience is actually Thumbnail carousel, not Thumbnails 👀

katycobb · 2021-12-07T22:21:08Z

@LucasLacerdaUX oof, good question. @duygukalaycioglu let's chat about this!

katycobb · 2021-12-07T23:21:52Z

@LucasLacerdaUX I connected with @duygukalaycioglu: we'll update the help text to Thumbnails is the default mobile layout.

No change to the mobile thumbnail default needed!

LucasLacerdaUX · 2021-12-08T02:51:14Z

@katycobb Not sure if I understand? The help text looks the same as before? 👀

LucasLacerdaUX · 2021-12-08T03:00:46Z

svinkle · 2021-12-08T18:38:47Z

@LucasLacerdaUX

Clicking a thumbnail will always announce that the image is loaded.

Confirmed. New issue being this text can be found when navigating via virtual cursor. If you can set a timeout to clear #GalleryStatus after a couple seconds that'd be great.

The Gallery Viewer title is now applied to the media-gallery component via aria-label and role="region" instead of being an <h2>

Confirmed.

If thumbnails are enabled and we're using desktop viewport, role="presentation" is applied to the <ul> and all <li> in the Gallery Viewer.

Confirmed on desktop and mobile for the thumbnail view. Although, now that I'm aware of the swipe feature on mobile (see note below) I'd say this would apply to desktop only. Is this possible?

aria-describedby was added to thumbnail buttons, linking them to the img element.

Confirmed.

Each media type now has its own index.

Confirmed, though image type seems to start at 0. Let's start at 1.

Firefox - Windows High Contrast mode fixed by styling the ::after pseudoelement instead of the button :)

Confirmed. New issue being the focus ring is placed below and to the left of the larger image in the gallery viewer. Seems to be images only. Please review.

Unfortunately, this seems to be caused by YouTube's video player.

Thanks for looking into this. It seems to be true for hosted, non YouTube video as well.

This would limit the current UX as it would no longer allow mobile users to navigate through the media via swipe gestures.

Ah, no worries. I didn't realize the swipe feature existed. With this context in mind, ignore my original comment.

katycobb · 2021-12-08T23:19:51Z

Hi @LucasLacerdaUX: Melissa, D and I connected and aligned on the behavior of this section and have made some content updates we think will better describe the settings. Let me know if this makes sense given your feedback and knowledge of the section!

Remove help text under Desktop layout
Rename Mobile thumbnails field label to Mobile layout
Update options under Mobile layout to: Show thumbnails and Hide thumbnails

cc: @duygukalaycioglu and @melissaperreault

LucasLacerdaUX · 2021-12-09T04:23:08Z

@svinkle

Demo links

Store
Editor

Confirmed. New issue being this text can be found when navigating via virtual cursor. If you can set a timeout to clear #GalleryStatus after a couple seconds that'd be great.

I've followed the aria-hidden => true pattern we've used in other components. Does that work? Let me know if you would prefer setting its content to empty instead.

Confirmed on desktop and mobile for the thumbnail view. Although, now that I'm aware of the swipe feature on mobile (see note below) I'd say this would apply to desktop only. Is this possible?

Updated. It now checks if the component is using a non stacked layout (thumbnails or thumbnails carousel) and if the viewport width is >750px.

Confirmed, though image type seems to start at 0. Let's start at 1.

Fixed! It now starts at 1 for every media type.

Confirmed. New issue being the focus ring is placed below and to the left of the larger image in the gallery viewer. Seems to be images only. Please review.

Is this issue restricted to when we are using VoiceOver / another assistive technology? Or does it also happen with the browser's regular focus ring? This is what the (browser's) keyboard focus ring looks like for me:

However, VoiceOver definitely shows an additional ring around the button element:

This duplicated focus ring also happens on the product card, which I based this solution on. However, in that case, we have the product title inside the full-size link (the one with that uses the ::after trick):

Do you have any ideas on how I could solve this for product images without breaking High Contrast Mode? If I set the button to wrap the entire image, we'll end up with the same issue we originally had before applying the ::after.

Thanks for looking into this. It seems to be true for hosted, non YouTube video as well.

Oh, you're right! That's why I was unable to get it fixed for YouTube 🤦 We had a .focus() when initializing ANY deferred media (models, videos and external videos).

Fixed. The focus now remains on the thumbnail regardless of the media type you select.

@katycobb Thanks for the update! Looks perfect to me now!

Demo links

Store
Editor

svinkle · 2021-12-09T16:09:32Z

@LucasLacerdaUX

I've followed the aria-hidden => true pattern

Confirmed. 👍

It now checks if the component is using a non stacked layout

Looks good.

Is this issue restricted to when we are using VoiceOver

Seems to be Safari only, when using Tab.

Screenshot

Dawn product page. Product image in gallery view features small focus ring in bottom left corner.

The focus now remains on the thumbnail regardless of the media type you select.

Confirmed.

LucasLacerdaUX · 2021-12-15T14:41:37Z

Seems to be Safari only, when using Tab.

Should be fixed now! Thanks, @svinkle.

Store
Editor

LucasLacerdaUX · 2021-12-15T14:43:10Z

PR is ready for final reviews.

Thumbnails / image srcset will be updated later this morning, but I would suggest to review everything else in the meantime :)

cc @tyleralsbury @ludoboludo @duygukalaycioglu

duygukalaycioglu

Looking great!!

ludoboludo

Looking very nice 👌

Left a few comments of things I noticed. Nothing major though 🤔

assets/section-main-product.css

ludoboludo · 2021-12-15T15:40:12Z

sections/main-product.liquid

+          "label": "t:sections.main-product.settings.gallery_layout.options__2.label"
+        },
+        {
+          "value": "thumbnail_slider",


I wonder if we should had an info here. But it might be overkill and self explanatory anyway:

Good question! I think it is self explanatory but a good candidate for help doc precisions.

assets/component-slider.css

assets/global.js

ludoboludo · 2021-12-15T16:32:01Z

assets/section-main-product.css

+  .product--thumbnail .product__media-gallery,
+  .product--thumbnail_slider .product__media-gallery,
+  .product--stacked .product__info-container--sticky {
+    display: block;
    position: sticky;


One thing I notice here with position sticky on the media gallery is that it makes it a bit annoying to get to see the video: video

Hum, this is interesting. A few things come to mind:

This is not only impacting videos but any media in a landscape ratio

This is not something that was concerning with the stacked layout because we had more vertical visuals to view

Since merchants can deactivate the Enable sticky header setting and the other one, I am not too concerned. That behaviour is not meant for all instances and this is the reason why the settings exist.

Our defaults set merchants for success if they remain with the Stacked layout.

assets/section-main-product.css

sections/main-product.liquid

snippets/product-thumbnail.liquid

LucasLacerdaUX · 2021-12-15T18:54:28Z

@ludoboludo and @tyleralsbury Image sizes have been updated for both main media, deferred-media posters and thumbnails :)

I've also addressed all of Ludo's comments about code. This should be ready for a final review.

ludoboludo · 2021-12-15T19:07:48Z

sections/main-product.liquid

+    }
+
+    removeListSemantic() {
+      this.elements.viewer.slider.setAttribute('role', 'presentation');


I do sometimes get an error in the console but I think it only happens in the theme editor before you hit Save: screenshot

assets/global.js

assets/section-main-product.css

sections/main-product.liquid

ludoboludo

Noted maybe one thing that could be changed in the PR. The vertical alignment of the thumbnail slider buttons. Otherwise didn't notice any other issues.

sections/main-product.liquid

ludoboludo · 2021-12-16T17:08:25Z

assets/media-gallery.js

+        }
+        if (!this.elements.thumbnails || this.dataset.desktopLayout === 'stacked') {
+          activeMedia.scrollIntoView({behavior: 'smooth'});
+        }


I kinda feel like it's nice to have it scroll back to the top of the media being opened. But I guess it's weird to keep focus on the thumbnail button and potentially have it outside of the viewport 🤔

This can be a follow up conversation anyway. Not a blocker and quick fix if needed.
video

ccing @melissaperreault for future conversation on this.

assets/component-slider.css

tyleralsbury · 2021-12-16T19:24:40Z

assets/media-gallery.js

+if (!customElements.get('media-gallery')) {
+  customElements.define('media-gallery', class MediaGallery extends HTMLElement {


This is clever. Using this approach we might even be able to inline some of our smaller custom elements. Keeping this one in the back of my mind for asset management ideas.

tyleralsbury

Looks good - I gave it some tests and looked again at the code. 🚢

LucasLacerdaUX changed the title ~~[Draft] Product Media Layout updates~~ Product Media Layout updates Nov 25, 2021

LucasLacerdaUX marked this pull request as ready for review November 25, 2021 05:49

LucasLacerdaUX commented Nov 25, 2021

View reviewed changes

locales/en.default.json Show resolved Hide resolved

LucasLacerdaUX requested review from melissaperreault and svinkle November 25, 2021 05:55

ludoboludo self-requested a review November 25, 2021 18:47

ludoboludo reviewed Nov 25, 2021

View reviewed changes

tyleralsbury self-requested a review November 26, 2021 15:18

tyleralsbury reviewed Nov 26, 2021

View reviewed changes

LucasLacerdaUX mentioned this pull request Nov 30, 2021

Slideshow section #840

Merged

10 tasks

melissaperreault requested a review from duygukalaycioglu December 2, 2021 18:13

duygukalaycioglu requested a review from katycobb December 3, 2021 19:27

duygukalaycioglu suggested changes Dec 3, 2021

View reviewed changes

LucasLacerdaUX mentioned this pull request Dec 8, 2021

Check if Product Modal exists before defining it #978

Open

LucasLacerdaUX force-pushed the product-media-layout-updates branch from a3349c4 to 23b1e24 Compare December 13, 2021 16:51

LucasLacerdaUX requested a review from ludoboludo December 13, 2021 19:40

LucasLacerdaUX force-pushed the product-media-layout-updates branch from 18d5eee to 56a370e Compare December 14, 2021 21:03

Fix label on slideshow buttons

73bd504

LucasLacerdaUX requested review from ludoboludo and tyleralsbury December 15, 2021 14:39

duygukalaycioglu previously approved these changes Dec 15, 2021

View reviewed changes

ludoboludo reviewed Dec 15, 2021

View reviewed changes

LucasLacerdaUX added 2 commits December 15, 2021 12:08

Media sizes update

95bc6c0

More reviewer comments. Thanks Ludo :D

7a4cdfd

LucasLacerdaUX dismissed duygukalaycioglu’s stale review via 7a4cdfd December 15, 2021 17:44

ludoboludo reviewed Dec 15, 2021

View reviewed changes

snippets/product-thumbnail.liquid Show resolved Hide resolved

LucasLacerdaUX added 2 commits December 15, 2021 13:48

Update thumbnail sizing

6ee8ba0

Add description to snippet

704c010

LucasLacerdaUX requested a review from ludoboludo December 15, 2021 18:53

ludoboludo reviewed Dec 15, 2021

View reviewed changes

sections/main-product.liquid Outdated Show resolved Hide resolved

LucasLacerdaUX added 3 commits December 16, 2021 01:00

Fix arrows... again

8d8f2c2

Fix featured product

3290a46

Check for slider before adding role=presentation

3a162d9

ludoboludo previously approved these changes Dec 16, 2021

View reviewed changes

ludoboludo dismissed their stale review via 3a162d9 December 16, 2021 18:00

Remove margin bottom

dcf41e5

ludoboludo approved these changes Dec 16, 2021

View reviewed changes

tyleralsbury reviewed Dec 16, 2021

View reviewed changes

tyleralsbury approved these changes Dec 16, 2021

View reviewed changes

LucasLacerdaUX merged commit 639d839 into main Dec 16, 2021

LucasLacerdaUX deleted the product-media-layout-updates branch December 16, 2021 19:44

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

Product Media Layout updates (Shopify#937)

216876f

		if (!customElements.get('media-gallery')) {
		customElements.define('media-gallery', class MediaGallery extends HTMLElement {

Product Media Layout updates #937

Product Media Layout updates #937

Conversation

LucasLacerdaUX commented Nov 23, 2021 • edited Loading

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucasLacerdaUX Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svinkle commented Nov 26, 2021 • edited Loading

tyleralsbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svinkle commented Nov 29, 2021 • edited Loading

svinkle commented Nov 29, 2021

svinkle commented Nov 30, 2021

duygukalaycioglu left a comment • edited by LucasLacerdaUX Loading

Choose a reason for hiding this comment

katycobb commented Dec 7, 2021 • edited Loading

LucasLacerdaUX commented Dec 7, 2021 • edited Loading

katycobb commented Dec 7, 2021

katycobb commented Dec 7, 2021

LucasLacerdaUX commented Dec 8, 2021

LucasLacerdaUX commented Dec 8, 2021 • edited Loading

Accessibility updates

Fixed

Not addressed

New decisions

svinkle commented Dec 8, 2021 • edited Loading

katycobb commented Dec 8, 2021

LucasLacerdaUX commented Dec 9, 2021 • edited Loading

svinkle commented Dec 9, 2021

LucasLacerdaUX commented Dec 15, 2021

LucasLacerdaUX commented Dec 15, 2021

duygukalaycioglu left a comment

Choose a reason for hiding this comment

ludoboludo 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

melissaperreault Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

LucasLacerdaUX commented Dec 15, 2021 • edited Loading

Choose a reason for hiding this comment

ludoboludo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tyleralsbury left a comment

Choose a reason for hiding this comment

LucasLacerdaUX commented Nov 23, 2021 •

edited

Loading

LucasLacerdaUX Dec 8, 2021 •

edited

Loading

svinkle commented Nov 26, 2021 •

edited

Loading

svinkle commented Nov 29, 2021 •

edited

Loading

duygukalaycioglu left a comment •

edited by LucasLacerdaUX

Loading

katycobb commented Dec 7, 2021 •

edited

Loading

LucasLacerdaUX commented Dec 7, 2021 •

edited

Loading

LucasLacerdaUX commented Dec 8, 2021 •

edited

Loading

svinkle commented Dec 8, 2021 •

edited

Loading

LucasLacerdaUX commented Dec 9, 2021 •

edited

Loading

melissaperreault Dec 16, 2021 •

edited

Loading

LucasLacerdaUX commented Dec 15, 2021 •

edited

Loading