-
Notifications
You must be signed in to change notification settings - Fork 6
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
Patrick/add cart page #11
Conversation
src/components/CartItems.tsx
Outdated
return ( | ||
<ListGroup> | ||
{ products.map((product, i) => ( | ||
<ListGroup.Item key={i}> |
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.
Since this list could get resorted (based on the name sort), the key={i}
can lead to weirdness:
https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318
Might be time to create an ID for each product, or, if the title is unique, then use that.
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've used product.id
- thanks for the link.
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.
LGTM!
Hey @van-fs would you please review as well. |
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.
LGTM
This PR adds the view/edit cart page. Please see comment thread for a callout/question for review.