-
Notifications
You must be signed in to change notification settings - Fork 7
LidiiaStarshynova-w2-React #12
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?
LidiiaStarshynova-w2-React #12
Conversation
d35f801
to
32605cf
Compare
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 Lidiia
it is good, clean code structure. easy to understand your code.
About error handling and loading statement, there's also need to be handle in the home page.
const ProductDetail = () => { | ||
const { id } = useParams(); | ||
const [product, setProduct] = useState(null); | ||
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'
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.
I prefer the first option, because now for me it is more understandable code than types 2 and 3.
|
||
const fetchCategories = async () => { | ||
try { | ||
const response = await fetch('https://fakestoreapi.com/products/categories'); |
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.
Suggestion:
In other fetching function you add below at the begining
setLoading(true);
setError(null);
But you don't need it here.
What is the difference? Do you need to add them or no need to?
Try to pick one and explain why you choose
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.
In this case I do not need this piece of code, because CategoryList is the part of main page. It can not be loaded separately.
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.
setLoading(true); | ||
setError(null); |
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.
setLoading(true); | |
setError(null); |
I mean here, you don't need this,
since these states are already properly initialized when the component mounts.
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