Conversation
…warnings (id not being included in the tests)
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work practicing React! Let me know if you have any questions about the comments.
🟢 for react-chatlog
| onUpdateChatData: PropTypes.func | ||
| }; | ||
|
|
||
| export default ChatLog; No newline at end of file |
| function getDifferenceInYears(timeStamp) { | ||
| const today = new Date(); | ||
| const year = today.getFullYear(); | ||
|
|
||
| const pastDay = new Date(timeStamp); | ||
| const pastYear = pastDay.getFullYear(); | ||
|
|
||
| const difference = year - pastYear; | ||
| return difference; | ||
| } |
There was a problem hiding this comment.
Nice job implementing your own solution for determining the time elapsed since a message was sent. If you check the components directory, you'll see TimeStamp component for you to use.
You can see how we implemented finding the time elapsed
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time">{timeInYears}</p> |
There was a problem hiding this comment.
How would you refactor line 36 to use the custom TimeStamp component? What would you need to pass into the component as a prop?
| const updateChatData = (updatedMessage) => { | ||
| const messages = chatMessagesData.map((message) => { | ||
| if (message.id === updatedMessage.id) { | ||
| return updatedMessage; | ||
| } else { | ||
| return message; | ||
| } | ||
| }); | ||
|
|
||
| setChatMessagesData(messages); | ||
| }; |
There was a problem hiding this comment.
We use this method to update how a chat entry renders after someone likes an entry. To constrain the scope of this method so that it is only responsible for re-rendering a chat entry after it's been liked, we might refactor this method so that we're not returning the entire message.
updateChatData could become something like:
const updateHeart = (id) => {
const chatMessagesCopy = [...chatMessagesData];
for (let entry of chatMessagesCopy) {
if (entry.id === id) {
entry.heart = !entry.heart;
}
}
setChatMessagesData(chatMessagesCopy);
};If you do this, then you can update ChatEntry.js too so you only need to pass the prop id instead of the whole message object
| const onLikeButtonClick = () => { | ||
| const updatedMessage = { | ||
| id: props.id, | ||
| sender: props.sender, | ||
| body: props.body, | ||
| timeStamp: props.timeStamp, | ||
| liked: !props.liked | ||
| }; | ||
|
|
||
| props.onUpdateChatData(updatedMessage); | ||
| }; |
There was a problem hiding this comment.
If you refactor onUpdateChatData as I suggested in App.js, then your onLikeButtonClick() method only needs to modify the liked property of of a ChatEntry. If you follow this approach, you'd also need to introduce useState to keep track of a ChatEntry's like state.
The benefit of only changing the liked property is that when a user clicks on a like, they really are only changing liked. Therefore, we shouldn't let the onLikeButtonClick event handler change id, sender, body, timestamp (which is more editing power than this method really needs).
Reworking a solution so only liked is being affected will require you to do more than change a line or two of code, but it would best reflect what actually needs to happen. We should always prefer to only change the properties/data that need to be changed when we code.
| for (let i = 0; i < chatMessagesData.length; i++) { | ||
| let message = chatMessagesData[i]; | ||
| if (users.indexOf(message['sender']) === -1) { | ||
| users.push(message['sender']) | ||
| } | ||
| if (message['liked']) { | ||
| likesCount++; | ||
| } | ||
| } |
There was a problem hiding this comment.
Putting this logic into a method like calculateHeartCount and then calling that function within the paragraph tag would help make this file a little more organized and readable.
<p className="header">{calculateHeartCount()} 💙s</p>Having said that, your logic as it is right now would need to be reworked so that when the method is called it would return an integer
| if (users.length === 2) { | ||
| user1 = users[0].toUpperCase(); | ||
| user2 = users[1].toUpperCase(); | ||
| } |
There was a problem hiding this comment.
You could also directly hard code these user names into your h1 element and remove this logic here so line 44 would look like:
<h1>CHAT BETWEEN {{chatMessages[0].sender}} AND {{chatMessages[1].sender}} </h1>2 participants
I implemented the time-stamp feature before I saw the TimeStamp component, so I left it that way. I modified the tests to account for the change in emoji.