-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add variant picker sold out UI #105
Conversation
Thanks for the contribution, cfxd. Based on our code principles for Dawn, there are some issues with this Pull Request. See the contributing guidelines for the full document on code principles. In particular, this PR has an issue with the server rendered principle. The goal in Dawn, and in theme development going forward, is for Liquid to handle all of the rendering tasks and for complex client-side logic for rendering to be avoided. This PR brings up an interesting question, though - is there a way for the Shopify platform, through Liquid alone, to help with this sort of thing? An example would be something along the lines of having something available in the value in My impression is that right now the platform doesn't exactly provide what would be needed for this to be possible, but it feels like a potential road forward for this functionality to be implemented without needing to do so much heavy lifting within the JS. |
Right on, I figured that's what you were going for based on #45 and I just wanted to get this out into the universe somehow. I do actually believe it's possible using liquid and I'm working on it! |
2c00a25
to
c764e83
Compare
@tyleralsbury check it out, just forced a push with a refactor in Liquid. Some question marks but at least it's a mvc. |
I ported over the changes to my test branch and it seems to work pretty well. It seems to be some weird things happening "flimmering" when switching sizes fast (but not very realistic): @cfxd great work 🙌 I hope @tyleralsbury can help merge this ASAP with needed changes as this is very "basic" need of our store. |
Thanks @gregjotau. I believe the delay you're experiencing is due to the |
I pushed it live on: https://famme.no/products/long-sleeve?variant=37560099471534 It works good enough for us, but of course, before merging should get some attention and cleanup :) |
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.
Works fine on famme.no
@tyleralsbury would love a code review when you have a chance please! |
165b064
to
47d581f
Compare
@tyleralsbury (and @gregjotau ) I just merged this with latest changes and squashed the commits. Would love a code review please! |
I have used the code for some time now on famme.no, also up to date with the latest version/commits. So 👍 from me. |
While the Liquid based approach works, I feel Shopify should have helper to do that more easily: {% for option in product.options %}
{% for value in option.values %}
// APPROACH 1:
{% if value.has_available_variants %}
// This value (for instance XS) has at least one variants
{% endif %}
// APPROACH 2: this one would be more flexible and would also allow to simplify other use cases:
{% assign available_variants = value.variants | where: 'available' %}
{% endfor %}
{% endfor %} The second approach would allows us to simplify code when we need to retrieve for instance the URL of the first available variant for a given color. |
Hey @cfxd , sorry I haven't looped back around to this for so long. Things were busy and I also had a bit of time off. I'm gonna be looking at this directly as well as adding it to the list of community PRs for our internal theme developers to review. @bakura10 thanks for that suggestion - one of the things we definitely do want to do is bring these types of simplification ideas to our platform team so that theme development can be easier. |
Hi @cfxd , thanks again for your contribution I've played with this for a little while this morning and it overall works really well. I've noticed a hitch when dealing with unavailable products, though. Because an unavailable product doesn't have a variant ID, when the So for example, you can set up the following: Variant 1 - Available in "Blue", "Small"
The issue is that it takes a user action, clicking on a crossed out option, to reload the product info. Unfortunately, I don't know if there's an elegant way around this in a case where we've reached an Unavailable product by clicking on "Option 1", since we are relying on the current variant ID to update the UI. A video demonstrating this flow: Screen.Recording.2021-09-02.at.10.24.32.AM.mov |
@tyleralsbury I cannot seem to have noticed this on famme.no? |
It happens under some pretty specific circumstances. You need to have a situation something like this: Option 1 - Colour
So that when switching from |
Hmm, this is a complex problem. If I do what some propose and simply hide or disable the unavailable options, then in this scenario a shopper would be forever stuck on Blue / Small and they couldn't switch the color or the size. Even if they could only switch the color option, then you're right—the size re-rendering never occurs because the initial selection is not an existing configuration. Quite a conundrum indeed. |
I agree. I think crossing them out but still letting them be clickable is the right way to approach this, as you implemented it.
It's tricky for sure, given the current reliance on the variant ID. The fact that these variants don't exist means that they can't have an ID, so we have nothing to base the update on. This is the main reason that I'm not 100% sure that Liquid provides everything we need to make this work currently. My idea would be that we'd need the platform to have something like "each subsequent option has an awareness of the previous ones". Then we would be able to use the selected state of the first to determine the available choices for the second, and the combination of those two to determine the third. This would also mean that we would need to have URL parameters for each of them, so that when we query the URL for the page, we would be able to say which of the choices were selected for each option. So instead of it relying on the variant to determine which options are supposed to be selected, it could rely on the options themselves. So, going back to this table.... Option 1 - Colour
When the page is loaded, "Red / Small" would be selected, so the page would load with Medium crossed out. This is already possible, but now let's say we changed the colour to "Blue", then there's no associated variant so currently the page has nothing to reference with regards to how to properly update. With this proposal, we'd have a property on Option 2 (in this case Size) that would have an awareness of the current selection of Option 1 and could be handled. It would be something like There is a big assumption here that I'm not 100% sure about, but it's the fact that the options are actually sequential, or in other words, that the platform even has a concept of "Colour is the first option, Size is the second option". I don't know if it does. Some downsides...
|
Hello, Any updates on this? Did anyone solve this problem? Thanks! |
@tyleralsbury What are your thoughts about involving some JS on this so that it can work in a modern way? |
Second that. It is consistent with the "javascript as needed" philosophy. Basically: What we have now is really bad, I would call it a critical bug even, to have such basic functionality missing. |
@cfxd Thanks so much for this. I will just make sure that all possible variants exist as out of stock products when I have more than one option avoiding the no variant id issue because IMO I'd rather have unavailable variants show as out of stock and greyed out then give the customers no signals. |
I still think we need some platform improvements to make it easier to determine directly in the Liquid... this way the logic is not handled in the JS and can be defined by a single source of truth. What do you think of this proposal? #105 (comment) Do you see any issues with the idea? Any edge cases we've not considered? |
@tyleralsbury for SEO I would say it is no problem, as you say as long as the canonical is set to the product url without parameters. A platform change like that would take a lot of time I suspect, so a temporary solution with JS to make Dawn palatable is necessary IMO fast. |
Any update here? This is actually hurting pretty bad. Seems to be totally broken. https://famme.no/products/sample I thought it always worked, but it might be because I added another kind of option setup, not sure why it is not updating properly now @cfxd on this product. Hopefully it is not more products |
Hey @gregjotau - it is definitely on our radar to take a look at this but is not currently slotted as a priority. We still need to sort out whether or not an approach that leverages a platform change is necessary to facilitate this. If we are implementing a feature like this, we want to make sure that it can be done in a strongly principled, well reasoned, and performant way. |
@tyleralsbury any update on this? We do not want to have custom code for this as it requires more time when merging new changes in. |
Unfortunately, we still do not have an update on this. Sorry about that. We will let you know when we do. |
@tyleralsbury any new year resolutions to fix this one? :) |
Are there any pending platform changes being worked upon that fixes this? 🙏 Would be great if one checked that, since if no changes is being worked upon, maybe accept some JS soultion. |
Does anyone have a JS solution as a fork btw? (that is bullet proof and addresses the concerns of @tyleralsbury above) |
@tyleralsbury @ludoboludo any update on this, we have a bug related to the sold out UI from this branch that is annoying: respiro-ecommerce/dawn-famme#46 So we really need this built in and solid. I would argue this is much more important than basically everything else like mega menu (which is also awesome btw!) and small UI fixes. |
@cfxd do you have a JS fix on another branch which is "bullet proof"? Would love to know if you have researched this more 🙏 |
@gregjotau @mohinht check #1407 |
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.
Sold Out variant picker
I guess this one can be closed since 1407 duplicates it? |
Yes, you can.
Van: greg taube ***@***.***>
Verzonden: dinsdag 28 juni 2022 06:46
Aan: Shopify/dawn ***@***.***>
CC: Kees Dubbelaar ***@***.***>; Comment ***@***.***>
Onderwerp: Re: [Shopify/dawn] Add variant picker sold out UI (#105)
I guess this one can be closed since 1407 duplicates it?
—
Reply to this email directly, view it on GitHub<#105 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AY6IPG6CPMQ6KWGS4GBSAO3VRJ7RPANCNFSM474F6Z2Q>.
You are receiving this because you commented.Message ID: ***@***.******@***.***>>
|
I'll close this PR since we have another one open and being reviewed. 👍 |
Add variant picker sold out UI, just a fist iteration to get the ball rolling
Why are these changes introduced?
Fixes #45 (Variant picker).
What approach did you take?
Enable/disable options as available, including an option to allow disabled combinations to remain enabled for use with third party stock notification apps.
Demo links
Password:
fleomo
Checklist