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.

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

changing color does not change the correct position in the slider #1805

Open
gregjotau opened this issue Jun 16, 2022 · 7 comments
Open

changing color does not change the correct position in the slider #1805

gregjotau opened this issue Jun 16, 2022 · 7 comments

Comments

@gregjotau
Copy link

In 99% av cases, the color pictures will be placed after eachother:

  1. 4 black pictures
  2. 4 pictures of red leggings
  3. 4 pictures of blue leggings
  4. etc.

Thus when changing variant, the position in the slider should be changed to the first available color image, not just change the picture.

Screenshot 2022-06-16 at 14 27 18

@gregjotau
Copy link
Author

gregjotau commented Jun 16, 2022

@ludoboludo , can you give a short assessment if you think this is an easy fix and where to fix? (thanks 🙏)

@ludoboludo
Copy link
Contributor

Hey @gregjotau, so the issue is with how we push that variant image in the front of the image list instead of scrolling to it right ?
And I'm guessing you're setup not to hide variant images or are you ?

If variant images are hidden, then I think it makes sense for it to work the way it currently does.

If variant images aren't hidden, I can see how it can be problematic, specifically when you rely on the order of the images to provide a certain experience to your users.

There could be a way to scroll to the image instead of prepending it to the list 🤔 This happens both in the updateMedia() and setActiveMedia(mediaId, prepend) functions.

So you would probably need to check what is the current product page setup (thumbnail slider or not, hidden variants or not) and then trigger a scroll event instead of the ul that has the thumbnails.

I can see how it could be nice to have but I'm not sure it would be prioritized over the stuff we currently have going on 🤔

Let's keep that issue open though 👍 Thanks for bring it up.

@gregjotau
Copy link
Author

Hey @gregjotau, so the issue is with how we push that variant image in the front of the image list instead of scrolling to it right ?

Yes.

With regards to hiding variant images based on color, that would be awesome, but the problem is that you have 4 pictures per color:

image

As you can see the "hide other" does not work for us.

Are there any plans to let images be linked to variants in such a way that hiding non-selected color-variants is trivial? Right now we need to create something custom like a naming convention or alt text to do some css magic.

@ludoboludo
Copy link
Contributor

I think for this what makes the most sense would be for it to be supported in the admin. Having a way to link multiple images to one variant. I'm not sure we're going to focus our team's efforts on creating a work around in our theme as we will need to find a logic that could work for most merchant and easy to understand which kinda seem tricky and hacky.

I've heard of this frustration before, not only from you and it's something we're aware of. I don't have all the knowledge necessary or context to be able to tell you what are the limitations for us to add this sooner rather than later though 😬

@gregjotau
Copy link
Author

gregjotau commented Aug 13, 2024

@ludoboludo any update on this one? Would be good if "scroll position" changes to the variant image, instead of pushing it to the front.

If it will not be prioritized, could you give pointers for how to most easily modify this in the code and if there has been any changed to how this is handled or if the description you gave above is still valid? (FYI. we use thumbnails)

@ludoboludo
Copy link
Contributor

Hey @gregjotau 👋

Yeah the description I gave above still seem accurate 👍 setActiveMedia(mediaId, prepend) and updateMedia(html, variantFeaturedMediaId) would be the place to edit.

You could prevent some of the logic in updateMedia() to run probably if you detect a data attribute telling you variant images aren't hidden. You can try a scrollIntoView() by selecting the element with the the right dataset.mediaId 🤔

@gregjotau
Copy link
Author

gregjotau commented Aug 16, 2024

e25b5fb5de9b8acc6a5030104bddaf5be930e6d1

example solution (developed by a third party), works, but probably not the best solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants