-
Notifications
You must be signed in to change notification settings - Fork 7
Ruba week2 react #15
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
base: main
Are you sure you want to change the base?
Ruba week2 react #15
Conversation
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.
Hi Ruba!! you did a GOOD job,
- Clean error and loading handling and API URL logic
- Good use of React Router's Link
since you quite advanced, I want to have a discussion with you
I really enjoyed reviewing your work!:)
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.
Praise:
- Excellent error state implementation
- Clean try-catch-finally structure, loading and error states handling
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.
Nice-to-have:
Remove unused files
|
||
const ProductList = ({ activeCategory }) => { | ||
const [products, setProducts] = useState([]); | ||
const [loading, setLoading] = useState(true); |
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.
Discussion:
Here are three types of UI state handling, do you think there is any difference, and which one you prefer?
// 1.
const [loading, setLoading] = useState(true);
const [error, setError] = useState(null);
// 2.
const [status, setStatus] = useState({ loading: true, error: null, success: false });
// 3.
const [status, setStatus] = useState('loading'); // 'loading' | 'error' | 'success'
@@ -0,0 +1,44 @@ | |||
import React, { useEffect, useState } from "react"; |
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.
import React, { useEffect, useState } from "react"; | |
import { useEffect, useState } from "react"; |
Nice-to-have:
When importing a React component, you can skip 'import React' for modern React (17+)
Reference
<div className="product-detail"> | ||
<img src={product.image} alt={product.title} className="product-image" /> | ||
<div className="product-info"> | ||
<h2 className="product-title">{product.title}</h2> |
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.
<h2 className="product-title">{product.title}</h2> | |
<h1 className="product-title">{product.title}</h1> | |
Suggestions:
Since ProductDetail is a new page, it is better to use h1
Discussion: |
Assignment Link
https://mystored.netlify.app
Assignment Requirements ✅
https://fakestoreapi.com/products/categories
-> To get all the categorieshttps://fakestoreapi.com/products
orhttps://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./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 thereact-router-dom
package and add the routing to your app regardless.Comments Rules
• Should be considered for implementation
• Optional enhancements
• Asking for clarification or exploring different solutions
• Encouraging positive patterns