React2-Week1#5
Conversation
villads-valur
left a comment
villads-valur
left a comment
There was a problem hiding this comment.
Hey Sathi 👋
Good job on the implementation! It looks like you achieved everything needed for this task!
It would be great if you add a bit of description and possibly some screenshots to the PR description, that helps me as reviewer alot!
Let me know if you have any questions at all.
| @@ -0,0 +1,20 @@ | |||
| import React from "react"; | |||
| import styles from "./Meal.module.css"; | |||
| export default function Meal({ meal }) { | |||
There was a problem hiding this comment.
Issue(style): I would suggest writing out all of the props you need to pass to the component for it to render properly. Passing an object like this will make it re-render everytime the parent component does, as the reference to the object is not stable. This will cause unnecessary computations which is detrimental to performance.
| async function fetchMeals() { | ||
| try { | ||
| const response = await fetch(api("/meals")); | ||
| if (!response.ok) { | ||
| throw new Error("Failed to fetch meals"); | ||
| } | ||
| const data = await response.json(); | ||
| setMeals(data); | ||
| } catch (error) { | ||
| console.error("Error fetching meals:", error); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion(performance): I would move the definition of this function out of the useEffect. If you define it inside the useEffect, it would be defined everytime the useEffect is called - this is unnecessary computation which could cause performance issues.
| const data = await response.json(); | ||
| setMeals(data); | ||
| } catch (error) { | ||
| console.error("Error fetching meals:", error); |
There was a problem hiding this comment.
Issue: Instead of putting the error in the console, i would suggest saving the error in a state variable and using that to display some proper message or feedback to the user.
| {meals.length === 0 ? ( | ||
| <p>No meals found</p> | ||
| ) : ( | ||
| <div className={styles.grid}> | ||
| {meals.map((meal) => ( | ||
| <Meal key={meal.id} meal={meal} /> | ||
| ))} | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Style: I would suggest breaking this ternary statement into a proper if-else statement instead. Where each branch of the if-else statement returns the content based on the different state. This makes it way clearer to read and reason what is returned by your component.
SathiC1990
force-pushed
the
React-2-Week-1
branch
from
466c2fa to
95ce069
Compare
|
Hi @villads-valur Thank you for your time reviewing my code .And |
2 participants
#Meal-Card Component