Hide variant images setting #158

ludoboludo · 2021-07-08T21:31:21Z

Why are these changes introduced?

Fixes #30
Fixes #191

What approach did you take?

I used CSS to hide variant images as mentioned in the issue. I had to also add some JS to deal with it on mobile where I needed to tweak how the total of slides was calculated.
So I'm creating a new array that regroup all li elements that have a clientWidth > 0. This is my way for not including hidden elements (using display: none;) within the array we use to calculate our totalPages.

Other considerations

One thing I'm noticing as I was playing with it, is that when you change the variant, it doesn't scroll you back up on the new image. I see in the JS: window.setTimeout(() => { parent.scroll(0, 0) }); but it doesn't seem to do anything. Video.

Demo links

Store
Editor

Testing

Go to the Editor linked above, to the product page (Shoes > Louise Slide Sandal) and click on the Product information heading and check the Hide inactive variants box.

Checklist

tyleralsbury

Quick suggestion. Will finish review afterwards.

assets/slider.js

sections/main-product.liquid

kmeleta

Still testing, but thought I'd leave a couple quick notes.

One thing I'm noticing as I was playing with it, is that when you change the variant, it doesn't scroll you back up on the new image. I see in the JS: window.setTimeout(() => { parent.scroll(0, 0) }); but it doesn't seem to do anything.

Seems like this line will successfully reset the horizontal scrolling on mobile when the slider is displayed. But on desktop, the scrollable element isn't the parent element of the media, it's the document/window.

assets/slider.js

melissaperreault

One thing I'm noticing as I was playing with it, is that when you change the variant, it doesn't scroll you back up on the new image.

From a UX perspective, it would be expected to scroll back up when the buyer interacts with the variant. If they have the interest to explore the variant list and that merchants assigned a media to it, it's relevant in the purchase flow to have focus on the right visual.

Other than that, looks good to me! 🎉 🎉

kmeleta

This looks good Ludo. Changing sizes now doesn’t scroll the featured image into view but color variant changes do. Good work getting that included.

On mobile though I notice a couple inconsistencies that I'll describe here, but I'm fine investigating them as a separate issue. They seem to be related to how the slider interacts with the variant picking in general, not necessarily a direct result of this PR.

Changing the variants still often doesn’t scroll to the top. This seems to mostly happen when the hide variant setting is disabled OR when it is enabled but you scrolled the slider to another image. https://screenshot.click/15-08-bzap0-dhhkb.mp4
The slider counter also counts down as the scroll position is reset to 0 upon changing the variant. This isn't a big deal but it'd be nice to try and fix. https://screenshot.click/15-12-v284p-uh7qr.mp4
I have also seen the slider lose track of it's position, which is kind of weird and hard to reproduce reliably https://screenshot.click/15-43-thbkj-8acf5.mp4

assets/variants.js

LucasLacerdaUX

Tested and it works as expected. I have a question about the expectation for modals, though.

assets/slider.js

sections/main-product.liquid

LucasLacerdaUX · 2021-07-15T20:27:50Z

locales/en.default.schema.json

@@ -1273,6 +1273,9 @@
        "enable_sticky_info": {
          "label": "Enable sticky product information on large screens"
        },
+        "hide_variants": {
+          "label": "Hide other variants’ media when a variant is selected"


If the expectation is to have all variant media visible inside the modal, does this setting naming still makes sense? My expectation is that turning this on will hide other variants' media EVERYWHERE.

The expectation is to hide everywhere when the setting is true, I believe it was the case when I tested if I recall. Otherwise it was overlooked, when this setting is enabled, it applies to all devices! 🙌

Suggested change

"label": "Hide other variants’ media when a variant is selected"

"label": "Hide other variants’ media after selecting a variant"

Hey @gretchen-willenberg the reason why we didn't see this line of text is because the PR was not merged yet. I suggested the change based on your audit! Confirming this is a setting label, so no period at the end of the sentence.

ludoboludo · 2021-08-05T16:16:43Z

@LucasLacerdaUX & @kmeleta, I've made a few changes to address some of your comments:

the scroll into view works better now but still some issues specifically on Chrome. It seems to have more issues. Firefox and Safari seem to work just fine but on Chrome it won't always happen or scrollLeft properly 🤷 trying to see what I can find. I saw mentioned that scrollTo might work better but it didn't seem to help when I tested. (issue I found and this one though it might not be the exact same issue 🤔
I used a setTimeout for preventing the sticky header to show up. When I test on a slow 3g connection it still seemed to work fine. I wanted to use something different but concerned if I added an element to product__media-list as it would potentially mess with some CSS styling specificity. Curious to hear your thoughts though.

kmeleta

Wanted to share a couple thoughts on potential improvements, but I think it'd be nice to get this work merged despite some little scroll quirks.

assets/global.js

sections/header.liquid

LucasLacerdaUX

I believe the JS needs some refactoring. Let me know if you have any questions or if there is something I can add to make it clearer.

Overall, I feel like this interaction between VariantSelects and StickyHeader can be improved by removing the .product-section interface between the two and concentrating the scroll prevention logic in the StickyHeader component.

assets/global.js

sections/header.liquid

…StickyHeader

Co-authored-by: melissaperreault <melissa.perreault@shopify.com>

LucasLacerdaUX

Mostly nitpicks, but there are some code-design related questions / discussions that I'd like to hear your thoughts.

assets/global.js

LucasLacerdaUX · 2021-08-17T15:15:25Z

sections/header.liquid


+      document.addEventListener('preventHeaderReveal', this.hideHeaderOnScrollUp);


I'm not so sure about this preventHeaderReveal listener on a document level + use of bubbling, because that means the event is being propagated on multiple levels...

Sure, from my limited knowledge, it seems like the performance impact is probably insignificant. But from a code design perspective, I feel like this event should be dispatched directly to the sticky-header from VariantSelects so this StickyHeader JS can easily add the eventListeners to itself.

Thoughts?

Great idea. It sounds much cleaner. My interpretation of dispatchEvent was that I was setting the element receiving a change to say to the others Hey, something happened within me!. I've changed the approach now: c09d46d

sections/header.liquid

LucasLacerdaUX · 2021-08-17T15:23:50Z

sections/header.liquid

+      window.clearTimeout(this.isScrolling);
+
+      this.isScrolling = setTimeout(() => {
+        this.preventReveal = false;
+      }, 66);


Is this needed when if (this.preventReveal == false)? If not, we can include it inside if (this.preventReveal) below.

Also, where is this 66 timeout coming from? 👀

Done 👍 As for the 66 it was just something I used because of the article reference I was using.

sections/header.liquid

ludoboludo · 2021-08-18T18:57:41Z

@LucasLacerdaUX, @kmeleta ready for another review. I've requested the translations in the mean time.

kmeleta

Looking real nice now! I just see one little thing, then I feel like we're gtg.

assets/global.js

kmeleta

LucasLacerdaUX · 2021-08-19T15:12:04Z

assets/global.js

+    const modalContent = document.querySelector(`#ProductModal-${this.dataset.section}`);
+    const newMediaModal = modalContent.querySelector( `[data-media-id="${this.currentVariant.featured_media.id}"]`);
    const parent = newMedia.parentElement;
+    if (parent.firstChild == newMedia) return;
+    modalContent.prepend(newMediaModal);


I just realized that this modalContent element is not the right one.

#ProductModal-${this.dataset.section} points to the external modal, not its content wrapper.

This is causing the element to be prepended to the wrong container:

LucasLacerdaUX

commit 399626d Author: tairli <tairli@yahoo.com> Date: Mon Aug 23 04:47:25 2021 +0930 Update product-form.js commit 330e6b4 Merge: 79467e4 7a2f34f Author: tairli <tairli@yahoo.com> Date: Mon Aug 23 04:45:54 2021 +0930 Merge remote-tracking branch 'upstream/main' into main commit 7a2f34f Author: Lucas Lacerda <37168033+LucasLacerdaUX@users.noreply.github.com> Date: Thu Aug 19 15:55:39 2021 -0400 Add featured product section (Shopify#261) commit 9789ae1 Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Aug 19 15:52:08 2021 -0400 Update 2 translation files (Shopify#449) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit d83f16f Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Aug 19 15:51:51 2021 -0400 Update 7 translation files (Shopify#448) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit ce0e5ec Author: Ludo <ludo.segura@shopify.com> Date: Thu Aug 19 14:46:05 2021 -0400 Hide variant images setting (Shopify#158) commit d3b44e7 Author: Ludo <ludo.segura@shopify.com> Date: Thu Aug 19 10:29:50 2021 -0400 Update font style for Default preset (Shopify#435) * Update font style for Default preset * updated default in settings_schema commit 2643bab Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Aug 19 10:29:34 2021 -0400 Update 15 translation files (Shopify#439) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit e9a2fd4 Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Thu Aug 19 10:02:16 2021 -0400 Update 4 translation files (Shopify#440) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit 9ea0392 Author: Ludo <ludo.segura@shopify.com> Date: Wed Aug 18 17:19:16 2021 -0400 404 continue shopping style (Shopify#419) commit 6c2128a Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Wed Aug 18 15:14:33 2021 -0600 Update price sale badge spacing (Shopify#430) commit a6d25cf Author: Kai <KaichenWang@users.noreply.github.com> Date: Wed Aug 18 11:13:22 2021 -0400 Adjust global focus styles for inputs, selects and collection filter UI elements (Shopify#389) * Adjust global focus styles for inputs, selects and collection filter UI elements * Fix focus for filter drawer summary elements * Simplify focus for elements that are always "focus-visible" when in focus * Adjust focus style of search submit button commit 002b6b0 Author: Ken Meleta <30790058+kmeleta@users.noreply.github.com> Date: Wed Aug 18 09:01:47 2021 -0600 Product card spacing and price polish (Shopify#407) * Adjust product card spacing and price styles * Fix unit price/sale badge spacing * Revert flex gap approach commit 6fa46b7 Author: Ludo <ludo.segura@shopify.com> Date: Wed Aug 18 10:54:58 2021 -0400 Update Preset default (Shopify#410) commit 341e051 Author: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> Date: Wed Aug 18 10:43:37 2021 -0400 Update 10 translation files (Shopify#427) Co-authored-by: translation-platform[bot] <34770790+translation-platform[bot]@users.noreply.github.com> commit 79467e4 Merge: 42cad96 789c3d9 Author: tairli <tairli@yahoo.com> Date: Wed Aug 18 19:26:00 2021 +0930 Merge branch 'Shopify:main' into main commit 42cad96 Merge: b5ddb38 2ba2725 Author: tairli <tairli@yahoo.com> Date: Tue Aug 17 22:06:29 2021 +0930 Merge branch 'Shopify:main' into main

ludoboludo marked this pull request as ready for review July 12, 2021 16:14

ludoboludo requested a review from tyleralsbury July 12, 2021 16:26

tyleralsbury reviewed Jul 12, 2021

View reviewed changes

assets/slider.js Outdated Show resolved Hide resolved

melissaperreault reviewed Jul 12, 2021

View reviewed changes

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

LucasLacerdaUX self-requested a review July 12, 2021 21:02

kmeleta reviewed Jul 14, 2021

View reviewed changes

assets/slider.js Outdated Show resolved Hide resolved

melissaperreault previously approved these changes Jul 15, 2021

View reviewed changes

ludoboludo dismissed melissaperreault’s stale review via 54c4861 July 15, 2021 15:51

kmeleta self-assigned this Jul 15, 2021

kmeleta previously approved these changes Jul 15, 2021

View reviewed changes

kmeleta removed their assignment Jul 15, 2021

LucasLacerdaUX reviewed Jul 15, 2021

View reviewed changes

assets/variants.js Outdated Show resolved Hide resolved

LucasLacerdaUX reviewed Jul 15, 2021

View reviewed changes

ludoboludo dismissed kmeleta’s stale review via 3bf5682 July 15, 2021 20:58

kmeleta mentioned this pull request Jul 21, 2021

Product page: variant images not scrolled into view #242

Closed

tyleralsbury mentioned this pull request Aug 2, 2021

Order of media Product Media Modal does not reflected the order displayed on the product page #223

Closed

ludoboludo added 6 commits August 5, 2021 11:53

Show currently selected variant image only

d007e4a

rebase part1

6cfb828

edit copy

702c71b

change if statement order

a422119

rebase part6

d60a827

rebase part8

a02eb3b

ludoboludo force-pushed the variant-img-setting branch from 1ba0a33 to a02eb3b Compare August 5, 2021 15:59

re add code tweaks to variant and slider in global.js

2163ff5

ludoboludo requested review from kmeleta and LucasLacerdaUX August 5, 2021 16:16

add scrollLeft into setTimeout

69fcff7

kmeleta self-assigned this Aug 9, 2021

kmeleta reviewed Aug 9, 2021

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

assets/global.js Outdated Show resolved Hide resolved

assets/global.js Outdated Show resolved Hide resolved

sections/header.liquid Outdated Show resolved Hide resolved

LucasLacerdaUX requested changes Aug 12, 2021

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

sections/header.liquid Outdated Show resolved Hide resolved

update modalContent selector

0b1a452

ludoboludo dismissed kmeleta’s stale review via 0b1a452 August 12, 2021 18:22

address reviewer's comment and focus on adding functionality for the …

409b004

…StickyHeader

ludoboludo requested review from LucasLacerdaUX and kmeleta August 12, 2021 19:28

Update locales/en.default.schema.json

87e4326

Co-authored-by: melissaperreault <melissa.perreault@shopify.com>

LucasLacerdaUX reviewed Aug 17, 2021

View reviewed changes

address reviewer's feedback

b3291eb

LucasLacerdaUX mentioned this pull request Aug 18, 2021

Add featured product section #261

Merged

5 tasks

update the dispatchEvent

c09d46d

ludoboludo requested a review from LucasLacerdaUX August 18, 2021 18:56

kmeleta reviewed Aug 19, 2021

View reviewed changes

assets/global.js Outdated Show resolved Hide resolved

remove code duplicate

6163377

ludoboludo requested a review from kmeleta August 19, 2021 14:49

kmeleta previously approved these changes Aug 19, 2021

View reviewed changes

LucasLacerdaUX requested changes Aug 19, 2021

View reviewed changes

fix modal selector

dfefb32

ludoboludo dismissed kmeleta’s stale review via dfefb32 August 19, 2021 16:14

ludoboludo requested review from LucasLacerdaUX and kmeleta August 19, 2021 16:14

Update 10 translation files

f7a7397

LucasLacerdaUX approved these changes Aug 19, 2021

View reviewed changes

kmeleta approved these changes Aug 19, 2021

View reviewed changes

ludoboludo merged commit ce0e5ec into main Aug 19, 2021

ludoboludo deleted the variant-img-setting branch August 19, 2021 18:46

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

Hide variant images setting (Shopify#158)

05fad1e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide variant images setting #158

Hide variant images setting #158

ludoboludo commented Jul 8, 2021 •

edited

Loading

tyleralsbury left a comment

kmeleta left a comment

melissaperreault left a comment

kmeleta left a comment •

edited

Loading

LucasLacerdaUX left a comment

LucasLacerdaUX Jul 15, 2021

melissaperreault Aug 2, 2021

melissaperreault Aug 16, 2021 •

edited

Loading

ludoboludo commented Aug 5, 2021

kmeleta left a comment

LucasLacerdaUX left a comment

LucasLacerdaUX left a comment

LucasLacerdaUX Aug 17, 2021

ludoboludo Aug 18, 2021

LucasLacerdaUX Aug 17, 2021

ludoboludo Aug 17, 2021 •

edited

Loading

ludoboludo commented Aug 18, 2021

kmeleta left a comment

kmeleta left a comment

LucasLacerdaUX Aug 19, 2021

ludoboludo Aug 19, 2021

LucasLacerdaUX left a comment

	"label": "Hide other variants’ media when a variant is selected"
	"label": "Hide other variants’ media after selecting a variant"


		document.addEventListener('preventHeaderReveal', this.hideHeaderOnScrollUp);

Hide variant images setting #158

Hide variant images setting #158

Conversation

ludoboludo commented Jul 8, 2021 • edited Loading

tyleralsbury 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

kmeleta left a comment • edited Loading

Choose a reason for hiding this comment

LucasLacerdaUX left a comment

Choose a reason for hiding this comment

LucasLacerdaUX Jul 15, 2021

Choose a reason for hiding this comment

melissaperreault Aug 2, 2021

Choose a reason for hiding this comment

melissaperreault Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

ludoboludo commented Aug 5, 2021

kmeleta left a comment

Choose a reason for hiding this comment

LucasLacerdaUX left a comment

Choose a reason for hiding this comment

LucasLacerdaUX left a comment

Choose a reason for hiding this comment

LucasLacerdaUX Aug 17, 2021

Choose a reason for hiding this comment

ludoboludo Aug 18, 2021

Choose a reason for hiding this comment

LucasLacerdaUX Aug 17, 2021

Choose a reason for hiding this comment

ludoboludo Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

ludoboludo commented Aug 18, 2021

kmeleta left a comment

Choose a reason for hiding this comment

kmeleta left a comment

Choose a reason for hiding this comment

LucasLacerdaUX Aug 19, 2021

Choose a reason for hiding this comment

ludoboludo Aug 19, 2021

Choose a reason for hiding this comment

LucasLacerdaUX left a comment

Choose a reason for hiding this comment

ludoboludo commented Jul 8, 2021 •

edited

Loading

kmeleta left a comment •

edited

Loading

melissaperreault Aug 16, 2021 •

edited

Loading

ludoboludo Aug 17, 2021 •

edited

Loading