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

Added add to cart behavior global setting #744

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion assets/product-form.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@ if (!customElements.get('product-form')) {
}

onSubmitHandler(evt) {
if (this.dataset.addToCartBehavior == 'go_to_cart') return;
Copy link
Contributor

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.

Copy link
Contributor Author

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".

Copy link
Contributor

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 🪙 🪙 🙂

Copy link
Contributor Author

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.

Copy link
Contributor

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 🤔


evt.preventDefault();
const submitButton = this.querySelector('[type="submit"]');
if (submitButton.classList.contains('loading')) return;
if (submitButton.classList.contains('loading')) return;

this.handleErrorMessage();
this.cartNotification.setActiveElement(document.activeElement);
Expand Down
21 changes: 21 additions & 0 deletions config/settings_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -328,5 +328,26 @@
"default": true
}
]
},
{
"name": "t:settings_schema.cart.name",
"settings": [
{
"type": "select",
"id": "add_to_cart_behavior",
"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"
}
],
Comment on lines +338 to +347
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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! 🙏

"default": "cart_notification",
"label": "t:settings_schema.cart.settings.add_to_cart_behavior.label"
}
]
}
]
14 changes: 14 additions & 0 deletions locales/en.default.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,20 @@
}
}
}
},
"cart": {
"name": "Cart",
"settings": {
"add_to_cart_behavior": {
"label": "Add to cart behavior",
tyleralsbury marked this conversation as resolved.
Show resolved Hide resolved
"cart_notification": {
"label": "Cart notification"
},
"go_to_cart": {
"label": "Go to cart"
tyleralsbury marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
},
"sections": {
Expand Down
2 changes: 1 addition & 1 deletion sections/featured-product.liquid
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@
{%- when 'buy_buttons' -%}
<div {{ block.shopify_attributes }}>
{%- if product != blank -%}
<product-form class="product-form">
<product-form class="product-form" data-add-to-cart-behavior="{{ settings.add_to_cart_behavior }}">
<div class="product-form__error-message-wrapper" role="alert" hidden>
<svg aria-hidden="true" focusable="false" role="presentation" class="icon icon-error" viewBox="0 0 13 13">
<circle cx="6.5" cy="6.50049" r="5.5" stroke="white" stroke-width="2"/>
Expand Down
2 changes: 1 addition & 1 deletion sections/main-product.liquid
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@
</noscript>
{%- when 'buy_buttons' -%}
<div {{ block.shopify_attributes }}>
<product-form class="product-form">
<product-form class="product-form" data-add-to-cart-behavior="{{ settings.add_to_cart_behavior }}">
<div class="product-form__error-message-wrapper" role="alert" hidden>
<svg aria-hidden="true" focusable="false" role="presentation" class="icon icon-error" viewBox="0 0 13 13">
<circle cx="6.5" cy="6.50049" r="5.5" stroke="white" stroke-width="2"/>
Expand Down