Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Simplify product-media-gallery snippet and consumers #3233

Merged
merged 7 commits into from
Mar 18, 2024
Merged

Simplify product-media-gallery snippet and consumers #3233

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e3343d7
Simplify featured product to use the same media gallery snippet as ma…
lhoffbeck Feb 2, 2024
f421978
Set images with the network response instead of by rendering hidden i…
lhoffbeck Feb 6, 2024
ce6ddd7
Removed unnecessary assignment
lhoffbeck Feb 6, 2024
ce60a3d
Update media refresh handling to minimize rerenders
lhoffbeck Feb 13, 2024
06b57af
Tweaks to fix JS error and modal content not updating when switching …
lhoffbeck Feb 28, 2024
300b6fb
Removed dead code
lhoffbeck Feb 28, 2024
5abaa7e
Update fallback selector
lhoffbeck Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 60 additions & 16 deletions assets/global.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,7 +972,6 @@ class VariantSelects extends HTMLElement {
this.toggleAddButton(true, '', true);
this.setUnavailable();
} else {
this.updateMedia();
this.updateURL();
this.updateVariantInput();
this.renderProductInfo();
Expand Down Expand Up @@ -1026,21 +1025,6 @@ class VariantSelects extends HTMLElement {
}
}

updateMedia() {
if (!this.currentVariant) return;
if (!this.currentVariant.featured_media) return;

const mediaGalleries = document.querySelectorAll(`[id^="MediaGallery-${this.dataset.section}"]`);
mediaGalleries.forEach((mediaGallery) =>
mediaGallery.setActiveMedia(`${this.dataset.section}-${this.currentVariant.featured_media.id}`, true)
);

const modalContent = document.querySelector(`#ProductModal-${this.dataset.section} .product-media-modal__content`);
if (!modalContent) return;
const newMediaModal = modalContent.querySelector(`[data-media-id="${this.currentVariant.featured_media.id}"]`);
modalContent.prepend(newMediaModal);
}

updateURL() {
if (!this.currentVariant || this.dataset.updateUrl === 'false') return;
window.history.replaceState({}, '', `${this.dataset.url}?variant=${this.currentVariant.id}`);
Expand Down Expand Up @@ -1114,6 +1098,64 @@ class VariantSelects extends HTMLElement {
if (productForm) productForm.handleErrorMessage();
}


updateMedia(html) {
const mediaGallerySource = document.querySelector(`[id^="MediaGallery-${this.dataset.section}"] ul`);
const mediaGalleryDestination = html.querySelector(`[id^="MediaGallery-${this.dataset.section}"] ul`);

const refreshSourceData = () => {
const mediaGallerySourceItems = Array.from(mediaGallerySource.querySelectorAll('li[data-media-id]'));
const sourceSet = new Set(mediaGallerySourceItems.map(item => item.dataset.mediaId));
const sourceMap = new Map(mediaGallerySourceItems.map((item, index) => [item.dataset.mediaId, {item, index}]));
return [mediaGallerySourceItems, sourceSet, sourceMap];
};

if (mediaGallerySource && mediaGalleryDestination) {
let [mediaGallerySourceItems, sourceSet, sourceMap] = refreshSourceData();
const mediaGalleryDestinationItems = Array.from(mediaGalleryDestination.querySelectorAll('li[data-media-id]'));
const destinationSet = new Set(mediaGalleryDestinationItems.map(({dataset}) => dataset.mediaId));
let shouldRefresh = false;

// add items from new data not present in DOM
for (let i = mediaGalleryDestinationItems.length - 1; i >= 0; i--) {
if (!sourceSet.has(mediaGalleryDestinationItems[i].dataset.mediaId)) {
mediaGallerySource.prepend(mediaGalleryDestinationItems[i]);
shouldRefresh = true;
}
}

// remove items from DOM not present in new data
for (let i = 0; i < mediaGallerySourceItems.length; i++) {
if (!destinationSet.has(mediaGallerySourceItems[i].dataset.mediaId)) {
mediaGallerySourceItems[i].remove();
shouldRefresh = true;
}
}

// refresh
if (shouldRefresh) [mediaGallerySourceItems, sourceSet, sourceMap] = refreshSourceData();

// if media galleries don't match, sort to match new data order
mediaGalleryDestinationItems.forEach((destinationItem, destinationIndex) => {
const sourceData = sourceMap.get(destinationItem.dataset.mediaId);

if (sourceData && sourceData.index !== destinationIndex) {
mediaGallerySource.insertBefore(sourceData.item, mediaGallerySource.querySelector(`li:nth-of-type(${destinationIndex + 1})`));

// refresh source now that it has been modified
[mediaGallerySourceItems, sourceSet, sourceMap] = refreshSourceData();
}
});
}

document.querySelector(`[id^="MediaGallery-${this.dataset.section}"]`).setActiveMedia(`${this.dataset.section}-${this.currentVariant.featured_media?.id}`);

// update media modal
const modalContent = document.querySelector(`#ProductModal-${this.dataset.section} .product-media-modal__content`);
const newModalContent = html.querySelector(`product-modal`);
if (modalContent && newModalContent) modalContent.innerHTML = newModalContent.innerHTML;
}

renderProductInfo() {
const requestedVariantId = this.currentVariant.id;
const sectionId = this.dataset.originalSection ? this.dataset.originalSection : this.dataset.section;
Expand Down Expand Up @@ -1145,6 +1187,8 @@ class VariantSelects extends HTMLElement {
`Volume-${this.dataset.originalSection ? this.dataset.originalSection : this.dataset.section}`
);

this.updateMedia(html);

const pricePerItemDestination = document.getElementById(`Price-Per-Item-${this.dataset.section}`);
const pricePerItemSource = html.getElementById(`Price-Per-Item-${this.dataset.originalSection ? this.dataset.originalSection : this.dataset.section}`);

Expand Down
22 changes: 9 additions & 13 deletions assets/media-gallery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ if (!customElements.get('media-gallery')) {
this.elements.thumbnails.querySelectorAll('[data-target]').forEach((mediaToSwitch) => {
mediaToSwitch
.querySelector('button')
.addEventListener('click', this.setActiveMedia.bind(this, mediaToSwitch.dataset.target, false));
.addEventListener('click', this.setActiveMedia.bind(this, mediaToSwitch.dataset.target));
});
if (this.dataset.desktopLayout.includes('thumbnail') && this.mql.matches) this.removeListSemantic();
}
Expand All @@ -28,21 +28,17 @@ if (!customElements.get('media-gallery')) {
this.setActiveThumbnail(thumbnail);
}

setActiveMedia(mediaId, prepend) {
const activeMedia = this.elements.viewer.querySelector(`[data-media-id="${mediaId}"]`);
setActiveMedia(mediaId) {
const activeMedia =
this.elements.viewer.querySelector(`[data-media-id="${mediaId}"]`) ||
this.elements.viewer.querySelector('[data-media-id]');
if (!activeMedia) {
return;
}
this.elements.viewer.querySelectorAll('[data-media-id]').forEach((element) => {
element.classList.remove('is-active');
});
activeMedia.classList.add('is-active');

if (prepend) {
activeMedia.parentElement.prepend(activeMedia);
if (this.elements.thumbnails) {
const activeThumbnail = this.elements.thumbnails.querySelector(`[data-target="${mediaId}"]`);
activeThumbnail.parentElement.prepend(activeThumbnail);
}
if (this.elements.viewer.slider) this.elements.viewer.resetPages();
}
activeMedia?.classList?.add('is-active');

this.preventStickyHeader();
window.setTimeout(() => {
Expand Down
9 changes: 4 additions & 5 deletions assets/section-featured-product.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,6 @@

.featured-product .product__media-item {
padding-left: 0;
width: 100%;
}

.featured-product .product__media-item:not(:first-child) {
display: none;
}

.featured-product .placeholder-svg {
Expand Down Expand Up @@ -53,6 +48,10 @@
.background-secondary .featured-product {
padding: 5rem;
}

.product--right .product__media-wrapper {
order: 2;
}
Comment on lines +52 to +54
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we originally had two was because of tab order. In our accessibility review, we came to the conclusion that using CSS order could result in a misleading tab order considering there are multiple elements in the Product Media column.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! Accessibility wise we wouldn't want to do that 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh great callout, I'll dig into this a bit more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay! I caught up with @svinkle (our internal a11y legend) about this. He called out that while it definitely would be preferable to have the visual order match the tab order, this likely wouldn't be a high-SEV issue and that the perf benefit may be a worthwhile tradeoff. We narrowed down on two approaches for when the image gallery is positioned on the right:

  • Skiplink: Continue using order to position and add a product info skiplink to the image gallery. Buyers landing on the page on desktop would tab first to the image gallery skiplink and (optionally) then to the product info, on mobile the behavior is unchanged from how it works today. Demo
  • Reposition content with JS: This would be a lot easier with a reactive UI framework but it's doable. This approach works but is brittle because it relies on the structure of the DOM. Demo

I put together a test store for each approach and Scott floated them past the Head of Accessibility Innovation at Fable:

She agreed that it’s a tough spot. In one hand, tab order is not being followed, but given the context that the user is in (navigating content related to the PDP context) and the close proximity of the content, she agreed that the skip link approach would be a viable option. She was sympathetic to the dev experience and worried about the other option could result in brittle code that could break at some point in the future without notice.
Let’s move ahead with the skip link option which should make things easier for you, result in better code, a more performant user experience, and a pretty good accessibility experience.

}

@media screen and (min-width: 990px) {
Expand Down
12 changes: 4 additions & 8 deletions assets/section-main-product.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@
.product__media-container .slider-buttons {
display: none;
}

.product--right .product__media-wrapper {
order: 2;
}
}

@media screen and (min-width: 990px) {
Expand Down Expand Up @@ -431,14 +435,6 @@ a.product__text {
}
}

.product__media-item.product__media-item--variant {
display: none;
}

.product__media-item--variant:first-child {
display: block;
}

@media screen and (min-width: 750px) and (max-width: 989px) {
.product__media-list .product__media-item:first-child {
padding-left: 0;
Expand Down
174 changes: 15 additions & 159 deletions sections/featured-product.liquid
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,6 @@

{%- liquid
assign product = section.settings.product

if section.settings.media_size == 'large'
assign media_width = 0.65
elsif section.settings.media_size == 'medium'
assign media_width = 0.55
elsif section.settings.media_size == 'small'
assign media_width = 0.45
endif
-%}

{% comment %} TODO: assign `product.selected_or_first_available_variant` to variable and replace usage to reduce verbosity {% endcomment %}
Expand All @@ -62,86 +54,24 @@
>
{%- endif -%}

{% assign variant_images = product.images | where: 'attached_to_variant?', true | map: 'src' %}

<section class="color-{{ section.settings.color_scheme }} {% if section.settings.secondary_background %}background-secondary{% else %}gradient{% endif %}">
<div class="page-width section-{{ section.id }}-padding{% if section.settings.secondary_background %} isolate{% endif %}">
<div class="featured-product product product--{{ section.settings.media_size }} grid grid--1-col gradient color-{{ section.settings.color_scheme }} product--{{ section.settings.media_position }}{% if section.settings.secondary_background == false %} isolate{% endif %} {% if product.media.size > 0 or section.settings.product == blank %}grid--2-col-tablet{% else %}product--no-media{% endif %}">
<div class="grid__item product__media-wrapper{% if section.settings.media_position == 'right' %} medium-hide large-up-hide{% endif %}">
<media-gallery
id="MediaGallery-{{ section.id }}"
role="region"
aria-label="{{ 'products.product.media.gallery_viewer' | t }}"
data-desktop-layout="stacked"
>
<div
id="GalleryViewer-{{ section.id }}"
class="product__media-list{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--fade-in{% endif %}"
>
{%- if section.settings.product != blank -%}
{%- if product.selected_or_first_available_variant.featured_media != null -%}
{%- assign media = product.selected_or_first_available_variant.featured_media -%}
<div class="product__media-item" data-media-id="{{ section.id }}-{{ media.id }}">
{% render 'product-thumbnail',
media: media,
position: 'featured',
loop: section.settings.enable_video_looping,
modal_id: section.id,
xr_button: false,
media_width: media_width,
media_fit: section.settings.media_fit,
constrain_to_viewport: section.settings.constrain_to_viewport
%}
</div>
{%- endif -%}
{%- liquid
assign media_to_render = product.featured_media.id
for variant in product.variants
assign media_to_render = media_to_render | append: variant.featured_media.id | append: ' '
endfor
-%}
{%- for media in product.media -%}
{%- if media_to_render contains media.id
and media.id != product.selected_or_first_available_variant.featured_media.id
-%}
<div class="product__media-item" data-media-id="{{ section.id }}-{{ media.id }}">
{% render 'product-thumbnail',
media: media,
position: forloop.index,
loop: section.settings.enable_video_looping,
modal_id: section.id,
xr_button: false,
media_width: media_width,
media_fit: section.settings.media_fit,
constrain_to_viewport: section.settings.constrain_to_viewport
%}
</div>
{%- endif -%}
{%- endfor -%}
{%- else -%}
<div class="product__media-item">
<div
class="product-media-container global-media-settings gradient{% if section.settings.constrain_to_viewport %} constrain-height{% endif %}"
style="--ratio: 1.0; --preview-ratio: 1.0;"
>
{{ 'product-apparel-1' | placeholder_svg_tag: 'placeholder-svg' }}
</div>
</div>
{%- endif -%}
</div>
{%- if first_3d_model -%}
<button
class="button button--full-width product__xr-button"
type="button"
aria-label="{{ 'products.product.xr_button_label' | t }}"
data-shopify-xr
data-shopify-model3d-id="{{ first_3d_model.id }}"
data-shopify-title="{{ product.title | escape }}"
data-shopify-xr-hidden
<div class="grid__item product__media-wrapper">
{%- if section.settings.product != blank -%}
{% render 'product-media-gallery', product: product, variant_images: variant_images, limit: 1 %}
{%- else -%}
<div class="product__media-item">
<div
class="product-media-container global-media-settings gradient{% if section.settings.constrain_to_viewport %} constrain-height{% endif %}"
style="--ratio: 1.0; --preview-ratio: 1.0;"
>
{% render 'icon-3d-model' %}
{{ 'products.product.xr_button' | t }}
</button>
{%- endif -%}
</media-gallery>
{{ 'product-apparel-1' | placeholder_svg_tag: 'placeholder-svg' }}
</div>
</div>
{%- endif -%}
</div>
<div class="product__info-wrapper grid__item{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--slide-in{% endif %}">
<product-info
Expand Down Expand Up @@ -429,12 +359,12 @@
render 'share-button', block: block, share_link: share_url
%}
{%- when 'variant_picker' -%}
{% render 'product-variant-picker',
product: product,
block: block,
product_form_id: product_form_id,
update_url: false
%}

Check warning on line 367 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L362-L367 

[NestedSnippet] Too many nested snippets

Check warning on line 367 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L362-L367 

[NestedSnippet] Too many nested snippets

Check warning on line 367 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L362-L367 

[NestedSnippet] Too many nested snippets
{%- when 'buy_buttons' -%}
{%- render 'buy-buttons',
block: block,
Expand Down Expand Up @@ -499,82 +429,8 @@
</a>
</product-info>
</div>
{%- if section.settings.media_position == 'right' -%}
<div class="grid__item product__media-wrapper small-hide">
{%- if section.settings.product != blank -%}
<media-gallery
id="MediaGallery-{{ section.id }}-right"
role="region"
aria-label="{{ 'products.product.media.gallery_viewer' | t }}"
data-desktop-layout="stacked"
>
<div
id="GalleryViewer-{{ section.id }}-right"
class="product__media-list{% if settings.animations_reveal_on_scroll %} scroll-trigger animate--fade-in{% endif %}"
>
{%- if product.selected_or_first_available_variant.featured_media != null -%}
{%- assign media = product.selected_or_first_available_variant.featured_media -%}
<div class="product__media-item" data-media-id="{{ section.id }}-{{ media.id }}">
{% render 'product-thumbnail',
media: media,
position: 'featured',
loop: section.settings.enable_video_looping,
modal_id: section.id,
xr_button: false,
media_width: media_width,
media_fit: section.settings.media_fit,
constrain_to_viewport: section.settings.constrain_to_viewport
%}
</div>
{%- endif -%}
{%- for media in product.media -%}
{%- if media_to_render contains media.id
and media.id != product.selected_or_first_available_variant.featured_media.id
-%}
<div class="product__media-item" data-media-id="{{ section.id }}-{{ media.id }}">
{% render 'product-thumbnail',
media: media,
position: forloop.index,
loop: section.settings.enable_video_looping,
modal_id: section.id,
xr_button: false,
media_width: media_width,
media_fit: section.settings.media_fit,
constrain_to_viewport: section.settings.constrain_to_viewport
%}
</div>
{%- endif -%}
{%- endfor -%}
</div>
{%- if first_3d_model -%}
<button
class="button button--full-width product__xr-button"
type="button"
aria-label="{{ 'products.product.xr_button_label' | t }}"
data-shopify-xr
data-shopify-model3d-id="{{ first_3d_model.id }}"
data-shopify-title="{{ product.title | escape }}"
data-shopify-xr-hidden
>
{% render 'icon-3d-model' %}
{{ 'products.product.xr_button' | t }}
</button>
{%- endif -%}
</media-gallery>
{%- else -%}
<div class="product__media-item">
<div
class="product-media-container global-media-settings gradient{% if section.settings.constrain_to_viewport %} constrain-height{% endif %}"
style="--ratio: 1.0; --preview-ratio: 1.0;"
>
{{ 'product-apparel-1' | placeholder_svg_tag: 'placeholder-svg' }}
</div>
</div>
{%- endif -%}
</div>
{%- endif -%}
</div>
{% render 'product-media-modal', product: product, variant_images: media_to_render %}
{% render 'product-media-modal', product: product, variant_images: variant_images %}
</div>
</section>

Expand Down Expand Up @@ -651,7 +507,7 @@
<script src="{{ 'media-gallery.js' | asset_url }}" defer="defer"></script>
{% endif %}

{% schema %}

Check notice on line 510 in sections/featured-product.liquid

View workflow job for this annotation

GitHub Actions / Theme Check Report

sections/featured-product.liquid#L510 

[SchemaJsonFormat] JSON formatting could be improved
{
"name": "t:sections.featured-product.name",
"tag": "section",
Expand Down
Loading
Loading