Ahmad_Zetouny-w2-React #15
Conversation
| @@ -0,0 +1,6 @@ | |||
| export default [ | |||
There was a problem hiding this comment.
You have missed a requirement from the README:
- The fake-data directory should not be a part of your project anymore
🔴
There was a problem hiding this comment.
Correct, I didn't delete the files. But it wasn't also used in the app
| throw Error(`Couldn't fetch the data: ${response.status}`); | ||
| } | ||
|
|
||
| const date = await response.json(); |
| ); | ||
|
|
||
| if (!response.ok) { | ||
| throw Error(`Couldn't fetch the data: ${response.status}`); |
There was a problem hiding this comment.
You might want to make your error message a little more user-friendly. The average user will not know what fetch means in this context, nor will they understand the 4xx or 5xx HTTP response code. 🔵
|
|
||
| return ( | ||
| <> | ||
| {error ? ( |
There was a problem hiding this comment.
This is called a nested ternary and it's generally not a great idea. Here's some info on conditional rendering in React: https://react.dev/learn/conditional-rendering 🟠
| @@ -0,0 +1,5 @@ | |||
| function Header({ title }) { | |||
There was a problem hiding this comment.
If a component is only rendering one JSX element with no extra logic it's not a useful component 🟠
| try { | ||
| let url = "https://fakestoreapi.com/products"; | ||
|
|
||
| if (filter != "") { |
There was a problem hiding this comment.
Always use === for strict equality. This is more reliable than == (loose equality). https://www.freecodecamp.org/news/loose-vs-strict-equality-in-javascript/ 🔴
| }, [filter]); | ||
|
|
||
| const productsList = products.map((product) => ( | ||
| <NavLink className="product" key={product.id} to={`/product/${product.id}`}> |
There was a problem hiding this comment.
Be sure to use the right linking component from react-router: https://medium.com/@adebimpeniola/link-vs-navlink-choosing-the-right-path-in-react-router-dom-8f85a744502c 🟠
| import Header from "./components/Header.jsx"; | ||
|
|
||
| function Product() { | ||
| let params = useParams(); |
There was a problem hiding this comment.
params does not change value (& is not an array) so we should use const for our variable declaration here. 🔴
| align-items: center; | ||
| justify-content: center; | ||
| gap: 2rem; | ||
| width: 70dvw; |
There was a problem hiding this comment.
Can you explain what dvw does and why you need it here instead of vw?
There was a problem hiding this comment.
dvw is more dynamic than vw, it calculates the actual viewport taking into concideration the browser UI
|
Thank you very much for your time Chris. I'll make sure to improve them or/and try different techniques in the next week's assignment |
https://react-week2-ahmad-zetouny.netlify.app/