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
Added add to cart behavior global setting #744
Conversation
cc @melissaperreault for alignment on the phrasing. I'm not sure if what I put is ideal. |
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.
Thank you Tyler! I added my first impression but would like to take the time to reflect on it before accepting any of my suggestions. I'll keep you posted very soon!
Nice ! regarding the naming I think the convention in all themes until now was to call the setting « Cart type » and the values being « Page » and « Drawer ». It may be nice to keep the same to ensure compatibility with previous documentation. |
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.
Noticed an issue with error messaging. Not sure if it's related to the PR.
@@ -10,9 +10,11 @@ if (!customElements.get('product-form')) { | |||
} | |||
|
|||
onSubmitHandler(evt) { | |||
if (this.dataset.addToCartBehavior == 'go_to_cart') return; |
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.
I think we could move this right after the this.handleErrorMessage();
so we still notify the user on the product page if there isn't more of this product ?
Here is some errors I noticed on the cart while testing it: video. But Let me know in case it was just local issues.
There seem to be an issue coming from dynamic checkout buttons 🤔 on the cart page: screenshot.
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.
For the first issue with the cart error, I'm thinking it might be a local environment issue (or a bug that came from elsewhere) since I didn't change any of the actual cart JS.
For the second issue, I'm wondering if we should do anything to change that... my thought is, the merchant is looking to effectively take the user right to the cart page and if we were to show an error on the product page, then that would mean we're still making an AJAX call and then sending them there only if there's a success. This will slow things down a lot compared to what I'm doing right now which is effectively just submitting the form (basically the no-js
flow).
I do find it interesting that the cart can't auto-adjust though, but I wonder if that's a limitation of the Liquid API. Like when you go to the cart page, it would make sense for the Liquid to have an awareness that the total quantity is higher than the available quantity. Because again, if the Liquid doesn't have that available, it's another situation where we'd need to make the check on the cart page when the page load via AJAX and then make adjustments. This also goes against one of our theme development principles of "don't do DOM adjustments prior to user input".
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.
Yeah it's kind of an odd situation. I think that if I can't add the product to the cart because there is none left/I've added them all already, then don't take me to the cart. Let me know on the spot rather than make me wait and load a new page to tell me that 🤔
It's kind of like a frustrating flow there was with recaptcha where sometimes you'd need to go through captcha verification to just be told the email was already registered or something. Ideally if something is wrong or can't be done, don't make me wait and tell me right away.
My 🪙 🪙 🙂
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.
Yeah, the problem is just that it would cause a delay in all of the cases where the action is valid since we'd need to do a round-trip AJAX call and then redirect to the cart anyway. Since the checkout itself can correct this, I think it makes sense for us to leave this behaviour in because the alternative is too clunky.
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.
I think we need to add more logic in here then.
So that if this.dataset.addToCartBehavior == 'go_to_cart'
is true we run a check on the product quantity available before going to the cart page with a fetch 🤔
Thank you @bakura10 for the shared context and contribution! Seems like a very common practice that I observed in other themes as well. 👍 @tyleralsbury Let's follow the current pattern, I'll make the most recent suggestion to follow this hierarchy and removed the previous ones to avoid confusion:
Why |
Co-authored-by: melissaperreault <melissa.perreault@shopify.com>
Co-authored-by: melissaperreault <melissa.perreault@shopify.com>
"options": [ | ||
{ | ||
"value": "cart_notification", | ||
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.cart_notification.label" | ||
}, | ||
{ | ||
"value": "go_to_cart", | ||
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.go_to_cart.label" | ||
} | ||
], |
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.
"options": [ | |
{ | |
"value": "cart_notification", | |
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.cart_notification.label" | |
}, | |
{ | |
"value": "go_to_cart", | |
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.go_to_cart.label" | |
} | |
], | |
"options": [ | |
{ | |
"value": "go_to_cart", | |
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.go_to_cart.label" | |
}, | |
{ | |
"value": "cart_notification", | |
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.cart_notification.label" | |
} | |
], |
I would like to order the options alphabetically please! 🙏
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.
Thank you Tyler! I just revised the behaviour for both and here is my additional feedback.
- When cart type is set to
Page
, is it possible to keep the loading spinner to give buyers the success feedback something is going on until the cart page load? We currently lost it and I got anxious clicking add to cart didn't succeed without it until the page finally loads. (Screen recording of what I mean)
UX noteTo consider the language update for the Cart type setting from this work effort #877 |
Closing this as I opened a new PR to tackle it 👍 #1638 |
Why are these changes introduced?
#732
What approach did you take?
Added a new category of global settings for cart and included a setting in there. I used a dropdown to allow for future expansion of options such as a minicart or cart drawer.
Note that I included it as a global setting rather than in each section so that the merchant can configure it broadly throughout their theme. We may want to add an overriding setting in each section that allows them more granular control if we see the need.
Demo links
Checklist