-
Notifications
You must be signed in to change notification settings - Fork 153
Snow Leopards - Alia Athman #113
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
base: main
Are you sure you want to change the base?
Conversation
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 effort! The wave 02 and 03 are missing functionality and the tests don't pass unfortunately. I left some specific comments below.
src/App.js
Outdated
const [messageData,setMessageData] = useState(chatMessages) | ||
|
||
const likeMessage = (id) => { | ||
setMessageData = messageData => messageData.map(message => { |
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.
Close! I think you meant to call the update function setMessageData
instead of trying to reassign it, like so:
setMessageData((messageData) =>
messageData.map((message) => {
if (message.id === id) {
return {
...message,
liked: !message.liked,
};
} else {
return message;
}
})
);
Note the callback function passed to setMessageData
and that you should set liked
by flipping the previous liked
state.
src/App.js
Outdated
{/* Wave 01: Render one ChatEntry component | ||
Wave 02: Render ChatLog component */} | ||
<ChatLog | ||
entries={chatMessages} |
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.
You should be passing messageData
down to the <ChatLog />
component. Otherwise updating the state will have no effect on the data you're currently passing (you're just passing the original data, not the data you added to the component state.
src/components/ChatLog.js
Outdated
import './ChatLog.css' | ||
|
||
|
||
// const entries = [ |
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.
Generally you should avoid committing commented out code when submitting a PR.
src/components/ChatLog.js
Outdated
// ] | ||
|
||
|
||
const ChatLog = ( entries ) => { |
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.
The React convention for naming the argument passed to a function component is props
.
src/App.js
Outdated
import React from 'react'; | ||
import './App.css'; | ||
import { useState } from 'react'; | ||
import ChatEntry from './components/ChatEntry'; |
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.
This import isn't being used, so we should delete this line. Extraneous imports should be omitted.
<button className="like">🤍</button> | ||
<p>{props.body}</p> | ||
<p className="entry-time"> <TimeStamp time={props.timeStamp}></TimeStamp> </p> | ||
<button className="like" onClick={() => props.onLikeMessage(props.id)}>🤍</button> |
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.
Since you're not passing the liked
prop into this component, you won't be able to set the red vs white heart emoji using that prop. If you did pass the prop, you could set that like
{props.liked ? <red-heart-emoji-goes-here> : 🤍}
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 updating this code! There's one substantial comment I left about not directly manipulating the DOM in a React app.
setMessageData(messageData => | ||
messageData.map(message => { | ||
if (message.id === id) { | ||
const button = document.querySelectorAll('button.like')[id-1] |
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.
We should avoid directly manipulating the DOM in this way when working in React. React is essentially purpose built to update the DOM when responding to state changes.
No description provided.