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 page #19

Merged
merged 18 commits into from
Sep 11, 2023
Merged

Cart page #19

merged 18 commits into from
Sep 11, 2023

Conversation

thelmaboamah
Copy link
Collaborator

@thelmaboamah thelmaboamah commented Sep 1, 2023

cart component with quantity counter and remove button for each item in the cart.
Covered issues: #2 #1 #3 #4 #6 #7 #24

@pearlescence-m pearlescence-m self-requested a review September 5, 2023 14:25
@@ -0,0 +1,3 @@
export default function Button({ action, children }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great job with the Cart modal - everything is neat so far and works as expected! However, one small comment I have is regarding the component props here and in the Modal component. Since we are using TypeScript, maybe it would be good to also show the types of passed props because without it they implicitly have a type of 'any'. Details on this page.

@thelmaboamah thelmaboamah changed the title Cart page minus styling Cart page Sep 7, 2023
Copy link
Owner

@larkinds larkinds left a comment

Choose a reason for hiding this comment

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

Great job! I left a few comments of items to address, once those are addressed this can be merged in!

client/src/App.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,20 @@
import { useEffect, useRef } from "react";

export default function Modal({ onClose, isOpen, children }) {
Copy link
Owner

Choose a reason for hiding this comment

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

Reminder to define the types of the props that you're importing

client/src/pages/Cart.tsx Outdated Show resolved Hide resolved
client/src/App.tsx Outdated Show resolved Hide resolved
client/src/components/Button.tsx Show resolved Hide resolved
<dialog ref={modalRef} className={className}>
<button
onClick={onClose}
style={{
Copy link
Owner

Choose a reason for hiding this comment

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

I tend to keep my inline styles to less than 3 - what do you think of using that as our standard for the project, and moving these styles into a css file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

moving these still out of here and passing the classes in as a prop instead.

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome, thanks!

@thelmaboamah
Copy link
Collaborator Author

@larkinds I think I've addressed all your requested changes. If there's nothing else, could you please approve this so I can merge this PR?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh my gosh, this icon is so cute!

Copy link
Owner

@larkinds larkinds left a comment

Choose a reason for hiding this comment

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

Looks great, really well done @thelmaboamah!

@thelmaboamah thelmaboamah merged commit f20dc0c into staging Sep 11, 2023
@pearlescence-m pearlescence-m deleted the cart-page branch September 15, 2023 16:19
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.

4 participants