Skip to content

Eli_D_Rajha-w2-React #10

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

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

Conversation

EliasRajha
Copy link

@EliasRajha EliasRajha commented Jun 24, 2025

  • The fake-data directory should not be a part of your project anymore
  • Your app will need to make 2 queries to the following endpoints:
    • https://fakestoreapi.com/products/categories -> To get all the categories
    • https://fakestoreapi.com/products or https://fakestoreapi.com/products/category/:selectedCategory -> To get the products. The API needs to do the filtering, not the frontend. Usually the amount of products will be too large to do the filtering on the frontend.
  • Your app needs to show that it is loading when waiting on the request to come back. You can test this by mimicing a slow connection in your browsers' developer tools
  • Your app needs to show an error message if the request failed
  • Your app needs to go to a detail page /product/:id whenever you click on the product card in the list. This should get the details from the endpoint: https://fakestoreapi.com/products/<id>. For now we won't add a navigation bar, the browsers 'back' button will do the trick. TIP: You will need to add the react-router-dom package and add the routing to your app regardless.
  • You need to deploy your app somewhere (using something like netlify) and put the link in your PR! Make it a different one than the previous week.

@EliasRajha
Copy link
Author

Live Netlify site for Week 2:
https://flourishing-bienenstitch-737ba8.netlify.app/

@ChingHsun
Copy link

ChingHsun commented Jun 29, 2025

  1. If the fetch fails, the product still gets displayed unexpectedly (ideally, it should only show an error message in that case)
  2. After reconnecting to the network, the error message should be gone too
    Screenshot 2025-06-29 at 22 52 49

Comment on lines 42 to 63
<Route
path="/"
element={
<div>
<h1>Ecommerce Shop</h1>

{error && <p>Error fetching data!</p>}
{loading ? (
<p>Loading...</p>
) : (
<>
<CategoryList
categories={categories}
selectedCategory={selectedCategory}
onSelectCategory={setSelectedCategory}
/>
<ProductList products={products} />
</>
)}
</div>
}
/>

Choose a reason for hiding this comment

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

It works fine as is, but splitting it into a MainPage component might help keep things more organized, especially as the app grows.

Comment on lines 3 to 4
<div style={{ display: 'flex', gap: '10px', flexWrap: 'wrap', marginBottom: '20px' }}>
{categories.map((cat, index) => (

Choose a reason for hiding this comment

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

I recommend using CSS Modules or Tailwind CSS instead of inline styles

Comment on lines 29 to 42
fetch(url)
.then((res) => {
if (!res.ok) throw new Error('Network response was not ok');
return res.json();
})
.then((data) => {
setProducts(data);
setLoading(false);
})
.catch(() => {
setError(true);
setProducts([]);
setLoading(false);
});

Choose a reason for hiding this comment

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

In this case, it is better to use finally to ensure the loading state with more readable and maintain reason

padding: '8px 12px',
}}
>
{cat}

Choose a reason for hiding this comment

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

Suggested change
{cat}
{category}

Using category instead of cat would be a small change in length, but it greatly improves readability. Especially collaborative with team project.

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

Successfully merging this pull request may close these issues.

2 participants