Alaa nasher w2 react #18
Conversation
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.2.11 to 5.4.6. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v5.4.6/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v5.4.6/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [braces](https://github.com/micromatch/braces) from 3.0.2 to 3.0.3. - [Changelog](https://github.com/micromatch/braces/blob/master/CHANGELOG.md) - [Commits](micromatch/braces@3.0.2...3.0.3) --- updated-dependencies: - dependency-name: braces dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
|
@Alaa2019-ml For week 3, please make sure you've checked out the latest version of the main branch. In your git history on this PR, I can see several old commits that were made by other contibutors (including me!) Your PRs should encompass only your own new code. |
crevulus
left a comment
crevulus
left a comment
There was a problem hiding this comment.
Hi Alaa,
Your app is working very well! Your code is a bit inconsistent - I've commented a few areas you could improve. Please see to those comments when you can.
| export const fetchApi = async (url) => { | ||
| const response = await fetch(url); | ||
| if (!response.ok) { | ||
| throw new Error(`HTTP error! Status: ${res.status}`); |
| const [error, setError] = useState(null); | ||
|
|
||
| useEffect(() => { | ||
| (async () => { |
There was a problem hiding this comment.
Can you explain why you have decided to use an IIFE here?
There was a problem hiding this comment.
I used an IIFE to keep the async logic scoped inside useEffect without creating a separate function that would only be called once.
| return ( | ||
| <> | ||
| <h1>{product.title}</h1> | ||
| <img src={product.image} alt={product.title} style={{ width: 200 }} /> |
There was a problem hiding this comment.
Use either inline styles or css stylesheets - not both! Consistency makes it easier for your colleagues to understand your code.
week2/project/ecommerce/src/App.jsx
Outdated
| import { Routes, Route } from "react-router-dom"; | ||
|
|
||
| function App() { | ||
| const [productsToShow, filterProductsToShow] = useState(null); |
There was a problem hiding this comment.
Make sure to follow the useState naming convention: stateVariable, and setStateVariable. You have some state variables in several files that don't follow this convention. Following conventions will help your colleagues undertand your code better.
week2/project/ecommerce/src/App.jsx
Outdated
| path="/" | ||
| element={ | ||
| <> | ||
| <h1>Products:</h1> |
There was a problem hiding this comment.
Storing route components inline will become very messy very quickly. Imagine you have 15 routes, and you add them all inline like this one. This file would very quickly become several hundred lines long and become difficult to manage. The dedciated component you've created for ProductDetails is a good pattern to follow.
|
I have updated the code. Thank you |
3 participants
https://cohort54-react-w2.netlify.app/