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

Cart item quantity basics #748

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Cart item quantity basics #748

wants to merge 7 commits into from

Conversation

dcsaszar
Copy link
Contributor

Next up: better support for changing the number of items in the cart view. Currently we only have DataFormNumberWidget which has worse UX than the +/- buttons. I used https://app.uxcel.com/courses/common-patterns/shopping-cart-best-practices-071 for inspiration.

@dcsaszar dcsaszar requested a review from apepper February 10, 2025 16:34
@apepper apepper self-assigned this Feb 11, 2025
Copy link
Contributor

@apepper apepper left a comment

Choose a reason for hiding this comment

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

FIRST-LOOK-REVIEW (5f82a27)

return product instanceof DataItem
? {
id: product.id(),
quantity: Number(item.get('quantity')),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you provide a fallback, e.g. to 1, if no quantity is given or NaN? E.g. someone added something to the cart, never touched it, we upgrade our code, now the quantity in the cart is undefined.

Comment on lines +55 to +59
return sum(
CartItem.all()
.take()
.map((item) => item.get('quantity')),
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need sum here? How about:

return CartItem.all()
    .take()
    .map((item) => item.get('quantity'))
    .filter((quantity) => typeof quantity === 'number')
    .reduce((acc, quantity) => acc + quantity, 0)

Comment on lines +218 to +229
<button
className="btn btn-sm btn-primary my-1"
onClick={async () => {
await subtractFromCart(product)
toast.info(cartRemovedMessage)
}}
>
<i className="bi bi-dash-lg px-0" />
<span className="d-none" aria-hidden>
-
</span>
</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make the button pass axe-core. Currently it fails with Buttons must have discernible text:
Screenshot 2025-02-11 at 11 26 47

const item = await load(() => findInCart(product))

if (item) {
item.update({ quantity: Number(item.get('quantity')) + 1 })
Copy link
Contributor

Choose a reason for hiding this comment

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

Number(item.get('quantity')) could be a NaN if no quantity exists (e.g. an earlier app version)

export function isInCart(product: ProductInstance): boolean {
if (!isUserLoggedIn()) return false // TODO: remove, once CartItem itself requires a login
export function quantityInCart(product: ProductInstance): number {
return Number(findInCart(product)?.get('quantity'))
Copy link
Contributor

Choose a reason for hiding this comment

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

Number(item.get('quantity')) could be a NaN if no quantity exists (e.g. an earlier app version)

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

Successfully merging this pull request may close these issues.

2 participants