-
Notifications
You must be signed in to change notification settings - Fork 77
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: when you edit the product qty to zero, the product won't be removed from the cart #542
Conversation
tblivet
commented
Aug 10, 2023
Questions | Answers |
---|---|
Description? | Correct the issue 536 |
Type? | bug fix |
BC breaks? | no |
Deprecations? | no |
Fixed ticket? | Fixes #536 |
Sponsor company | @PrestaShopCorp |
How to test? | You can refer to the issue |
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.
Looks good to me 👍
ping @ga-devfront for 2nd opinion
IMO it would be better to simply disallow changing quantity to 0. This can be done by mistake and then the product would have to be searched again. If a user wants to remove the product from the cart then the Remove button should be clicked. |
@SharakPL That was how it was before. It should throw an error message "Minimal quantity for sale is 1". Also, it's a bit incosistent. What does it do if minimum quantity for sale is higher than 1? 🤔 I am a bit torn apart for the expected behavior here. |
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.
@sallemiines could you please check again with product that has minimum order quantity higher than 1? It doesn't look like it's covered. |
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.
blocking as we need to check before merge
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.
Merging is blocked, I will remove the waiting for QA
label until the changes requests are resolved.
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 wanted to agree with @Hlavtox and @SharakPL, but I've just tested it on PS 8.1 with Classic, and it works like this:
The behavior should be as follows:
- If you change from 1 to 0 using arrows, the product is deleted.
- If you change from 1 to 0 using the input value, the product is deleted.
- If you have a product with a minimal quantity set to 2 and you change it to 1, you have an error.
Why does it work in Classic, and it doesn't work with Hummingbird?
What's the difference between Classic and Humminbird?
If you have 1 unit in your shopping cart and you change it to 0 (either by using arrows or input), the request goes to delete
action. In Hummingbird, it always goes to the update
action, which is wrong.
This PR almost fixes the issue but there's a problem with getting a minimal quantity which could be different than 1.
@tblivet Related function in Classic is inside: themes/classic/_dev/js/cart.js in
It looks that instead of checking minimal quantity you should only check whether the target value is equal to 0. |
Is there a place for improvements or does it have to be done like in Classic? |
@SharakPL what would be an improvement in your opinion? |
To be more clear on what each action does. Decreasing the amount shouldn't delete the product from the cart.
Or at least change [ - ] to [ X ] or [ trash_icon ] to indicate that next use of this button will remove it completely instead of just decreasing the amount. Especially useful if the minimum required amount is higher than 1. |
The one improvement I see in this change is that if one makes a mistake and changes the value below 1, it could save the cart. I've checked a few stores, and when there are arrows, changing below 1 is forbidden. There's no request, which could also be a good thing. Something to be decided by @PrestaShop/product-council :) To summarize:
|
Did you mean "below 1"? |
@SharakPL yes, I edited the message |
@RosaBenouamer There needs to be a number input, dropdown for selecting count is not good UX. :-) |
src/js/pages/cart.ts
Outdated
const targetItem = eventTarget.closest('.cart__item'); | ||
const targetValue = targetItem?.querySelector('.js-cart-line-product-quantity') as HTMLElement | null; | ||
|
||
if (targetValue && targetValue.getAttribute('value') === '1' && targetValue.getAttribute('min') === '1') { |
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.
Be careful the min quantity can also be 0 which should be handled like 1, meaning the feature is disabled there is no minimum
Except that I think the original behaviour from classic is consistent and should be used as a base as it's a common way to handle this in many e-commerce shops
tldr; let's stay iso functional
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 and one other comment you seem to be listening to the click event, so only the arrows clicking but the value can also be edited via keyboard, so you should either listen to the keydown
event as well or maybe easier simply the change
event?
Unless you need to listen to the specific event to cancel it when the value is not the minimum one?
Hello 👋 With @ga-devfront we have update the PR we now have this behavior :
|
No response from the people who blocked the PR, additions were made and the behavior seems to be the most common to web standards: |
if (targetValue) { | ||
if (eventTarget.classList.contains('js-increment-button')) { | ||
if (targetValue.dataset.mode === 'confirmation' && Number(targetValue.value) < 1) { | ||
if (removeButton) { | ||
removeButton.click(); | ||
} | ||
} | ||
} | ||
|
||
if (eventTarget.classList.contains('js-decrement-button')) { | ||
if (targetValue.getAttribute('value') === '1' && targetValue.getAttribute('min') === '1') { | ||
if (removeButton) { | ||
removeButton.click(); | ||
} | ||
} | ||
} | ||
} |
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.
if (targetValue) { | |
if (eventTarget.classList.contains('js-increment-button')) { | |
if (targetValue.dataset.mode === 'confirmation' && Number(targetValue.value) < 1) { | |
if (removeButton) { | |
removeButton.click(); | |
} | |
} | |
} | |
if (eventTarget.classList.contains('js-decrement-button')) { | |
if (targetValue.getAttribute('value') === '1' && targetValue.getAttribute('min') === '1') { | |
if (removeButton) { | |
removeButton.click(); | |
} | |
} | |
} | |
} | |
if (targetValue && removeButton) { | |
if (eventTarget.classList.contains('js-increment-button')) { | |
if (targetValue.dataset.mode === 'confirmation' && Number(targetValue.value) < 1) { | |
removeButton.click(); | |
} | |
} | |
if (eventTarget.classList.contains('js-decrement-button')) { | |
if (targetValue.getAttribute('value') === '1' && targetValue.getAttribute('min') === '1') { | |
removeButton.click(); | |
} | |
} | |
} |
The code can be simplified, the first if
avoid 2 ifs
.
All could go in an single if
but I wouldn't know how readable it would be.
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.
Hello @tblivet ,
LGTM ,
If you change from 1 to 0 using arrows, the product is deleted. ✔️
If you change from 1 to 0 using the input value, the product is deleted. ✔️
If you have a product with a minimal quantity set to 2 and you change it to 1, you have an error.
✔️
[but i think that the error message disappear too fast ]
Dashboard.Prestashopdev.mp4
It's QA approved ✔️
thank you !
I'm dismissing Fabien. His alert was listened