Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
🎉 Nice job! React is challenging and can take several projects to feel comfortable with. I've left comments throughout, so please take a look and let me know in Slack if there's anything you'd like to follow up on.
| const updateChat ={ | ||
| id : props.id, | ||
| sender : props.sender, | ||
| body : props.body, | ||
| liked : !props.liked, | ||
| timeStamp : props.timeStamp, | ||
| } | ||
| props.onUpdate(updateChat); |
There was a problem hiding this comment.
👀 This style of creating the new object to go in our list is how Learn showed working with the student attendance example. However, it turns out we don't need to do this when we send the id of the record we want to report has changed. The id passed there can come directly from props, as
props.onUpdate(props.id);Our local component here shouldn't need to know how to toggle the like status. Rather, this component should pass the id of the message whose like button was clicked to the callback, and that logic should be responsible for determining what the new liked status for the message should be. This would help ensure we have a single source of truth (the list of messages), from which we can draw the entire UI.
// in App.js
const handleLikeClicked = (id) => {
// copy the list of message stored in the App state
// copy the message indicated by the id
// flip the liked status for the message
// update the App state tracking the messages
};
// later in App.js
<ChatLog entries={messages} onLikeClicked={handleLikeClicked} />| props.onUpdate(updateChat); | ||
| } | ||
|
|
||
| const heartColor = props.liked ? '❤️': '🤍'; |
There was a problem hiding this comment.
👍 Nice calculation of the emoji to use based on the liked prop. We could even do this inline in the JSX, though I do like the clarity it brings to do it on its own and give the result a good name.
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p> {props.body} </p> | ||
| <p className="entry-time"><TimeStamp time = {props.timeStamp}></TimeStamp></p> |
There was a problem hiding this comment.
👍 Nice use of the TimeStamp component.
Stylistically, prefer to omit the spaces around = in JSX attributes. And JSX prefers using the /> syntax for closing empty components.
<p className="entry-time"><TimeStamp time={props.timeStamp} /></p>| body: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| onUpdate: PropTypes.func.isRequired |
There was a problem hiding this comment.
Because this proptype is marked isRequired, we're getting warnings in the tests, since the test author didn't know what you were going to call this property, and so it's absent from the props passed in during the tests. To avoid the warnings, we can leave off isRequired.
Ideally, we'd also include code to ensure that any "optional" values are actually present before trying to use them. For example, in the like click handler we could do something like
if (onUpdate) {
onUpdate(id);
}which would only try to call the function if it's not falsy (undefined is falsy).
| import React from 'react'; | ||
| import './ChatLog.css'; | ||
| import ChatEntry from './ChatEntry'; | ||
| import './ChatLog.css' |
| console.log(props.entries); | ||
| const chatComponents = props.entries.map((chat, index)=> { | ||
| const entriesData = [...props.entries]; | ||
| const totalLike = [...entriesData].reduce((a,b) => a + (b.liked? 1: 0), 0) |
There was a problem hiding this comment.
👍 Even though I'd recommend moving this logic, this is still a nice use of reduce to total up the number of liked messages in the chat.
| }; | ||
|
|
||
| ChatLog.propsTypes ={ | ||
| chatEntrys: PropTypes.arrayOf(PropTypes.shape({ |
There was a problem hiding this comment.
👍 Nice use of shape to describe the data the values within the array should have.
| @@ -1,8 +1,27 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
We can add the useState import here
import React, { useState } from 'react';|
|
||
| const [chatEntries, setChatsLike] = useState(chatMessages) | ||
|
|
||
| const updateLikeData = updatedChat =>{ |
There was a problem hiding this comment.
Rather than receiving a new message with it's like status flipped here, rewrite this to accept an id. The logic about how to copy the message and flip the liked status is better housed here, since the state must have all the information necessary to make a new message, whereas in complex systems, we may not pass all of the information need to make a new data element all the way down to a presentation component like ChatEntry.
| } | ||
| }); | ||
|
|
||
| setChatsLike(chats) |
There was a problem hiding this comment.
👀 Since the next version of the chat data depends on the current version, prefer using the callback style of setChatsLike
setChatsLike(chats => (
// logic to use current chats to make a new copy with the desired liked status flipped (the map logic above)
// return the new chats to update the state
))2 participants
No description provided.