-
Notifications
You must be signed in to change notification settings - Fork 117
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
Zoisite Yvett J #117
base: main
Are you sure you want to change the base?
Zoisite Yvett J #117
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.
Nice work wrapping this project up 🥳
I left a couple comments related to how you can lift the state of messages up from ChatEntry up to App.js. My point spans several comments so have a look at them together. Ultimately, we should use state to keep track of chatMessages, but we don't need it for keeping track of likes.
Let me know if you have questions about my review comments! You're done with core curriculum 💪
const likeMessage = () => { | ||
setLikedMessages(likedMessages => likedMessages + 1); | ||
}; | ||
|
||
const unlikeMessage = () => { | ||
setLikedMessages(likedMessages => likedMessages - 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.
You can actually keep track of the number of likes across all the messages without using state, thus we should prefer to not use state to reduce the complexity of the project.
Instead, you can create a function that iterates over all the chats and adds up the number of likes, something like:
const getHeartCount = () => {
return chatMessages.reduce((total, chat) => {
return chat.liked ? total + 1 : total;
}, 0)
};
// then on line 21, you can write this:
<h2>{getHeartCount()} ❤️s</h2>
This approach for calculating the total likes across all messages, would require you to keep track of the messages as state though and you'd need an event handler for updating when a message is liked. I'll leave more details about that in another comment.
{/* 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.
Instead of just passing in chatMessages from the messages.json file, we should use state as a single source of truth for keeping track of the data related to messages. Therefore, we should have a piece of state on line 8 like:
const [chatMessageData, setChatMessagData] = useState(chatMessages);
You'd also need an event handler that you can pass down to ChatEntry.js that would help you toggle the liked value on a single chat. It would be better for the logic about how to copy the message and change the liked status to be written in App.js
const updateEntryData = id => {
const entries = chatMessageData.map(entry => {
if (entry.id === id){
return {...entry, liked: !entry.liked};
}
else {
return entry;
}
});
setChatMessagData(entries);
}
<ChatLog | ||
entries={chatMessages} | ||
likeMessage={likeMessage} | ||
unlikeMessage={unlikeMessage} |
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.
Following my comment above about creating an event handler in app.js to help you keep track of a message's 'liked' status, you would need to pass that event handler from App to ChatLog and finally into ChatEntry.
<ChatLog
entries={chatMessageData}
updateEntryData={updateEntryData}
/>
<button className="like">🤍</button> | ||
<p>{body}</p> | ||
<p className="entry-time"> | ||
<TimeStamp time={timeStamp}/> |
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.
Nice job using the provided TimeStamp component
import PropTypes from 'prop-types'; | ||
|
||
const ChatEntry = (props) => { | ||
const ChatEntry = ( { sender, body, timeStamp, likeMessage, unlikeMessage } ) => { | ||
const [hearted, setHearted] = useState(false); |
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.
Don't need state here to keep track of a message's 'liked' status.
const toggleHeart = () => { | ||
setHearted(hearted => !hearted); | ||
hearted ? unlikeMessage(): likeMessage(); | ||
}; |
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.
Following my suggestion about passing down an event handler that can be used to change a message's 'liked' status, you would use that event handler in toggleHeart here:
const toggleHeart = () => {
props.updateEntryData(props.id);
};
ChatLog.propTypes = { | ||
entries: PropTypes.array.isRequired | ||
}; |
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.
Be sure to write out the full propTypes in the future to include shapeOf
id: PropTypes.number.isRequired, | ||
sender: PropTypes.string.isRequired, | ||
body: PropTypes.number.isRequired, | ||
timeStamp: PropTypes.string.isRequired, | ||
liked: PropTypes.bool.isRequired |
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.
propTypes is missing a couple values that are passed in from ChatLog to ChatEntry: likeMessage and unlikeMessage
hearted ? unlikeMessage(): likeMessage(); | ||
}; | ||
|
||
const heartedIcon = hearted ? '❤️' : '🤍'; |
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.
👍
const chatEntries = entries.map(entry => { | ||
return ( | ||
<ChatEntry | ||
id={entry.id} |
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 also use the id for the key prop since we're iterating over many entries to create many ChatEntrys:
<ChatEntry
id={entry.id}
key={entry.id}
// the rest of your props for your component
/>
No description provided.