-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
React1 week2/gayathri #240
base: main
Are you sure you want to change the base?
Conversation
Please ignore the week1 homework |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 👋🏻 I'm Elena your mentor reviewer for this assignment
{socialMediaLinks.map((social) => ( | ||
<SocialMediaItem key={social.url} {...social} /> | ||
))} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 💪🏻
const onAddOrRemovePlanet = (name, index) => { | ||
setSelectedPlanets((prevSelected) => { | ||
if (prevSelected.includes(name)) { | ||
return prevSelected.filter((planet) => planet !== name); | ||
} else { | ||
return [...prevSelected, { name }]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful, there's a bug here 🐛
You should return
return [...prevSelected, name]
Right now you are appending an object containing name
instead of just adding the name string, therefore you are not entering the if case.
This causes problems because:
The includes()
is looking for a string but comparing against objects
The isSelected
prop in PlanetCard
is using includes()
which won't work with objects
If you run the app and keep selecting the same planet you are just appending more and more objects to that array.
No description provided.