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

Homework react w3 christina #41

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

Conversation

christinaghazal
Copy link

No description provided.

@christinaghazal
Copy link
Author

..

@durw4rd durw4rd self-assigned this Jan 10, 2024
Copy link

@durw4rd durw4rd left a comment

Choose a reason for hiding this comment

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

Hi Christina,
Well done on your work and apologies for the late review!
I've added a few comments, mostly they are 'nice-to-have' recommendations aimed at making your code easier to understand. I'll leave it up to you if you want to implement those changes as the feedback can be also subjective and other people might have different views.
One thing I would like you to fix before I accept the work, though, is the product detail page - at the moment it's not working properly, possibly because you updated how the useFetch hook works.

Copy link

Choose a reason for hiding this comment

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

When creating custom hooks, I think it's better if the hook code is kept fairly generic and any component-specific logic is handled in the respective components. In the case of a useFetch hook like this, I would recommend passing only the URL as an argument. The category argument is not necessary if you move the logic to ProductList component. I'm not sure what the purpose of the noFavItems attribute is? When I deleted it, I didn't see any impact; maybe I'm missing some specific scenario for which you are using it?


const ProductDetailsPage = () => {
const { id } = useParams();
const { data: product, loading, error } = useFetch(`/${id}`);
Copy link

Choose a reason for hiding this comment

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

You will need to update the attributes passed to the useFetch hook for it to work correctly, at the moment the product details page doesn't show any content beyond the error message. I suspect you have updated the hook and forgot to update the component? :)

Comment on lines +7 to +37
const FavoritesPage = () => {
const { items } = useContext(FavContext);
//console.log("run");
const favListHasItems = items.length > 0;
const { data, loading, error } = useFetch({
url: "https://fakestoreapi.com/products",
noFavItems: !favListHasItems,
});

if (loading) {
return <h1>Loading...</h1>;
}

if (error) {
return <h1>Error!</h1>;
}

let content = <h2>Your favorite list is empty!</h2>;

if (favListHasItems) {
const favProducts = data.filter((product) => items.includes(product.id));
content = <FavList products={favProducts} />;
}

return (
<main>
<Navbar title="Favorites" />
{content}
</main>
);
};
Copy link

Choose a reason for hiding this comment

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

You can make the check for whether there are any favourite items more implicit like this:

const FavoritesPage = () => {
  const { items } = useContext(FavContext);
  const { data, loading, error } = useFetch("https://fakestoreapi.com/products");

  if (loading) {
    return <h1>Loading...</h1>;
  }

  if (error) {
    return <h1>Error!</h1>;
  }

  const favProducts = data.filter((product) => items.includes(product.id));

  return (
    <main>
      <Navbar title="Favorites" />
      {items.length > 0 ? <FavList products={favProducts} /> : <h2>Your favorite list is empty!</h2>}
    </main>
  );
};

This is not to say that your solution is incorrect. But in general if your code can be made shorter without making it more difficult to understand, that's preferable.


const FavContext = React.createContext({
items: [],
toggleFavProduct: (id) => {},
Copy link

Choose a reason for hiding this comment

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

I don't think the toggleFavProduct function is needed here.

@@ -0,0 +1,17 @@
import { useReducer } from "react";
import { favReducer, defaultFavState } from "./fav-reducer";
Copy link

Choose a reason for hiding this comment

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

Nit: for application of this size & complexity, it's not necessary to break the reducer into its own file, you could also leave it in here together with the provider.

Comment on lines +3 to +18
const favReducer = (state = defaultFavState, action) => {
if (action.type === "TOGGLE_FAV") {
const exitingId = state.items.findIndex((itemId) => itemId === action.id);
//console.log(exitingId);
let updatedItems;
if (exitingId >= 0) {
updatedItems = state.items.filter((itemId) => itemId !== action.id);
} else {
updatedItems = [...state.items, action.id];
}
return {
items: updatedItems,
};
}
return state;
};
Copy link

Choose a reason for hiding this comment

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

The code works just fine but seems a little inefficient. I think it's not necessary to explicitly declare the default empty array of favorite items. Unless you were planning to perhaps store the information in local storage and persist the state between page reloads?

The findIndex method could also be replaces with includes which seems like a better fit here (and you are using it already in your Favorites component!).

Moving things around a bit, you can avoid a few temporary variables - for example:

const favReducer = (favorites, action) => {
  if (action.type === "TOGGLE_FAV") {
    if (favorites.items.includes(action.id)) {
      return { items: favorites.items.filter((itemId) => itemId !== action.id) };
    } else {
      return { items: [...favorites.items, action.id] };
    }
  }
};

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