-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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.
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
Fix product slider on mobile #791
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested on my Google Pixel and it worked nicely.
I tried while on the first image and further down the line, it seems to always work. 🎉
Co-authored-by: Kai <KaichenWang@users.noreply.github.com>
6e9efbe
Fixed the single quote, should be good to go now for final approval 🙂 |
parent.scrollLeft = 0; | ||
parent.querySelector('li.product__media-item').scrollIntoView({behavior: 'smooth'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ludoboludo Sorry for revisiting this, but if we are to keep parent.scrollLeft = 0;
as a fallback, would it make sense to have it after scrollIntoView()
? This way on mobile if the behavior: 'smooth'
option is supported, the smooth scrolling will occur first and if it isn't supported, parent.scrollLeft = 0
will kick in. Having it the opposite way prevents the behavior: 'smooth'
from ever being effective on mobile? Thoughts?
parent.scrollLeft = 0; | |
parent.querySelector('li.product__media-item').scrollIntoView({behavior: 'smooth'}); | |
parent.querySelector('li.product__media-item').scrollIntoView({behavior: 'smooth'}); | |
parent.scrollLeft = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, in my testing I'm not really seeing a difference 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sweet 🙂 then I will just go ahead and merge this way 👌
Hey folks, when I test this fix on Dawn 2.3.0 (merchant's theme and fresh install in my test store | Chrome) the Did anyone else experience this happening with the fix above? |
Hey @okaykomputer is this a thing you can only experience in that version of Dawn ? |
Hey @ludoboludo , the issue does not replicate in 2.4.0 and after because (I'm presuming) the code for the product image slider was removed and redone using the code for the new slideshow section that was recently added in 2.4.0. |
Why are these changes introduced?
Fixes #665
What approach did you take?
There is some inconsistency for whatever reason when you're clicking a variant with an attached image on android devices. It's supposed to scroll it into the view, right after the image is pushed to the beginning of the product media list.
So i added another scrolling action:
parent.scrollLeft = 0;
.I kept
scrollIntoView
as I still need it on desktop and it's working fine there. My guess is that there is a hiccup when we're doingparent.prepend(newMedia);
and then trying to scroll that element into the view. Some of the timing might be off and it's messing the functionality of the slider.Other considerations
I tried to see if using
parent.firstChild
instead ofparent.querySelector(...
would help but it didn't change anything.Demo links
Testing
In order to test you'll need to check the preview from an android device. The issue wasn't happening using only the inspect tools.
You can search for the
sandal
product and click the different variant colours to see if there is any issue when it's supposed to scroll you to the variant image associated to the colour.Checklist