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

Moayad Mohammed - React - Week3 #22

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

Conversation

TheYodash
Copy link

@TheYodash TheYodash commented Sep 17, 2024

@crevulus crevulus self-assigned this Sep 17, 2024
Copy link

@crevulus crevulus left a comment

Choose a reason for hiding this comment

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

Hi Moayad,

I can't fully review this code because you're missing key requirements from the README. Please sort that out before you continue with your project and push to this PR again. Currently your week3 assignment is unfinished!

) : (
favourites.map((favouriteItem, id) => (
<li key={id} className="product-item">
<div className="favourite-icon" onClick={() => handleFavouriteClick(favouriteItem)}>

Choose a reason for hiding this comment

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

Currently this handleFavouriteClick method is triggered when you click anywhere on the item card. It has something to do with your HTML structure. Can you try moving the elelments in this card around so that the method is not triggered when you click the card? 🔴

import { Link } from 'react-router-dom';

const Favourites = () => {
const { favourites, removeFromFavourites } = useFavourites();

Choose a reason for hiding this comment

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

You're storing the whole item object in favourites. This is usually the best approach, but in this assignment we asked you to do soemthing else. From the README:

Your app should have a favourites page to list all of the products the user has favourited. You will need to perform multiple fetches as you only have the id of the favourites.

🔴

@@ -0,0 +1,12 @@
import * as React from 'react';

Choose a reason for hiding this comment

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

You're not using this file! You're using useFavourites from another file. You should delete it to avoid confusion. 🟠

}
}

const fetchProducts = async () => {

Choose a reason for hiding this comment

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

You missed something important from the readme:

If you did the same things as us then in the Products and ProductDetails pages you will have quite a lot of duplicate logic concerning the loading and error states of all those requests. Fix that by creating a useFetch custom hook.

🔴

@crevulus
Copy link

crevulus commented Oct 2, 2024

Hi Moayad,

Here are the comments that are as yet unresolved:

#2 (comment)
#11 (comment)
#11 (comment)
#11 (comment)

@crevulus
Copy link

Hi Moayad, I'm approving this PR because of the clean up efforts you've made. There is, however, one place where you're repeating yourself unnecessarily. Can you find it? 🤔 Hint: you made a reusable piece of code and reused it in some places, but not all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants