Moayad Mohammed - React - Week2 #11
Conversation
There was a problem hiding this comment.
Hi Moayad,
Good work on this week's homework. You are successfully calling for data and rendering it programmatically.
You have some changes that will need to be made around how you render components, though. You generally should avoid tightly coupling when you can - this is when one component depends on another in order to render: https://dev.to/gvegacl/understanding-coupling-in-react-practices-and-examples-1h1n
Please make the edits and push them to your PR.
| padding: 0.5rem .5rem; | ||
| background-color: #f1f1f1; | ||
| color: #333; | ||
| cursor: pointer; |
There was a problem hiding this comment.
If you're using the right HTML elements (like a and button), you won't need to set the cursor type. Also in this case, it's causing misleading UX. The whole category box has cursor: pointer, but only the text is actually clickable. This is a little frustrating for the user. 🟠
| } | ||
|
|
||
| .product-item { | ||
| flex: 1 1 300px; |
There was a problem hiding this comment.
This style is causing you issues. Using a flex-basis of 300px shouldn't be necessary for you. It's also causing the hitbox to be extra large. If you want your items to be nicely centered, try applying flex styles on the parent element (.products) instead of on the children. 🟠
| }, [id]); | ||
|
|
||
| return ( | ||
| error ? <h1>There was an error fetching product</h1> : |
There was a problem hiding this comment.
Here you've created a nested ternary. These are not great for the readability and maintainability of your code. What if there's another condition, e.g. if the network call is successful but the data is empty? Instead, try a rendering pattern like the one suggested in this article: https://blog.logrocket.com/ui-design-best-practices-loading-error-empty-state-react/ 🟠
|
|
||
| const categoryApiUrl = 'https://fakestoreapi.com/products/categories'; | ||
|
|
||
| const CategoryController = () => { |
There was a problem hiding this comment.
This is quite an odd component structure. You have ProductsController and Categories inside CategoryController. The categories and product should be in separate components, as you've correctly rendered below. Currently, if there's an error in the network call to fetch categories, it will cause the ProductsController not to render. Those two network calls should work independently from one another. 🔴
| const Categories = ({ categories, selectedCategory, handleCategoryClick }) => { | ||
| return ( | ||
| <div className="categories"> | ||
| { categories.length !== 0 ? ( |
There was a problem hiding this comment.
What if the network request returns 0 results, for whatever reason? Your data will not be "loading", but your app will show "Loading Categories...". You should instead use a loading state variable and call setIsLoading (or something similar) when the network call is being made. You will need to do this in all places where you do a network call, instead of relying on the length or existence of data to determine if loading is happening. 🔴
|
|
||
| return ( | ||
| errorFetch ? <h1>There was an error fetching products</h1> : | ||
| <Products products={products} selectedCategory={selectedCategory}/> |
There was a problem hiding this comment.
By adding this ProductsController you've managed to keep the Products component as "dumb" as possible - no network logic, just simple UI rendering. This is very smart component-based design. Well done.
| @@ -0,0 +1,59 @@ | |||
| * { | |||
There was a problem hiding this comment.
I suggest you add some mobile breakpoints, because at the moment you have some odd padding and margin on small screens - and small screens are more commonly used than big screens these days! 🟢
deployment link:
https://hyf-moayad-react-w2.netlify.app/