Dorelys w2 react #20
Dorelys w2 react #20
Conversation
week2/project/ecommerce/src/App.jsx
Outdated
| <Route | ||
| path="/" | ||
| element={ | ||
| <> |
There was a problem hiding this comment.
It's best practice to use a component here, rather than putting lots of JSX inline in the element prop. You can imagine if your site has 50 routes, that will be 50 implementations of JSX. This file would quickly become unmanageable! 🟢
| @@ -0,0 +1,31 @@ | |||
| #category-list { | |||
| margin: 40px auto; | ||
| } | ||
|
|
||
| .product-detail img { |
There was a problem hiding this comment.
It's better to apply a class to the img, p, and the h2, rather than targeting the image and title elements inside another class. We only really do this if we're importing a third party library and you need to access elements you can't add class names to. But in your case, you can control the class names on those elements. 🟠
| background-color: #fff; | ||
| padding: 20px; | ||
| border-radius: 10px; | ||
| box-shadow: 0px 4px 6px rgba(0, 0, 0, 0.1); |
There was a problem hiding this comment.
In some places you use hex codes and in others you use rgb. Choose one and stick to it in order to be more consistent. 🟢
| transition: transform 0.3s ease, box-shadow 0.3s ease; | ||
| } | ||
|
|
||
| .product-item:hover { |
| if (err.name !== "AbortError") { | ||
| setProducts([]); | ||
| setError("Failed to load products"); | ||
| setLoading(false); |
There was a problem hiding this comment.
You call setLoading(false) twice. Can you think of a way to only do it once? Hint: it is a method related to your try---catch logic. 🟠
|
|
||
| return ( | ||
| <div id="product-list" className="product-list"> | ||
| {loading && <Spinner />} |
There was a problem hiding this comment.
That's a lot of conditional inline logic! It will become hard to manage very quickly. you need to check if loading, if loading but not error, if error but not data etc.
Instead, it's better to do conditional logic above your main return statement.
if (loading) return ...
if (error) return ...
🟠
| const controller = new AbortController(); | ||
| const signal = controller.signal; |
There was a problem hiding this comment.
This is a very advanced pattern, and probably too complex for this kinf of application. I suggest you remove it and make this fetch call more similar to your fetch calls in other components. 🟠
| setLoading(false); | ||
| }) | ||
| .catch((err) => { | ||
| if (err.name !== "AbortError") { |
There was a problem hiding this comment.
Once you've made your change to remove AbortController you will not need this if line. 🟠
| }) | ||
| .then((data) => { | ||
| setCategories(Array.isArray(data) ? data : []); | ||
| setLoading(false); |
There was a problem hiding this comment.
In this implementation you call setLoading twice, but in ProductDetail you only call it once. Once is better! 🟠
There was a problem hiding this comment.
All requested changes have been implemented. Here is the link to the newly deployed version.
Here is the Link to the website