Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Nice job!
I made some notes about general approach, as well as about some warnings that occur when running tests (some of which we might think about how to address, some I think are more our fault).
| <button className="like">🤍</button> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time"> | ||
| <TimeStamp time={props.timeStamp} /> |
There was a problem hiding this comment.
👍 Good use of the supplied TimeStamp component.
| id: PropTypes.number.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| onUpdate: PropTypes.func.isRequired, |
There was a problem hiding this comment.
The ChatEntry tests only know about sender, body, and timeStamp. Marking any other properties as required causes warnings in the ChatEntry tests, as well as the ChatLog tests, since the LOG list in that test file also only includes sender, body, and timeStamp in the data. We could either relax the properties from being required (we should also make sure our components don't crash if non-required data is missing) or update the tests to pass all the data we need.
| }; | ||
| return <div className="chat-log">{getChatLogJSX(messages)}</div>; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Remember to add propTypes for the ChatLog component.
| return messages.map((message) => { | ||
| return ( | ||
| <ChatEntry | ||
| id={message.id} |
There was a problem hiding this comment.
We should add a key to any components that we render into a list. Here, the ideal key is the message id (key={message.id}), though the tests sadly don't pass a message id in with its sample data. 😭
For the tests to be happy, we might optimistically assume that the timestamps are unique (at least they're included in the test data!) and use those as the key. Or we could use the 2 parameter version of map, in which the index gets passed in as the second argument
messages.map((message, index) => { ...And use the index as the key. In general, this is the least preferable method and should be avoided if the data in the list can change (best option is a primary key from a database record.)
| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = (props) => { | ||
| const buttonEmoji = props.liked ? '❤️' : '🤍'; |
There was a problem hiding this comment.
Nice use of ternary for calculating which heart to show based on whether the entry is liked.
| timeStamp: props.timeStamp, | ||
| liked: !props.liked, | ||
| }; | ||
| props.onUpdate(updatedMessage); |
There was a problem hiding this comment.
I tend to prefer the approach of just sending the id to the update method. If we have complex objects, we might not always pass down everything needed to make a new instance down to the control that's displaying it. I also like to think of the presentational component as just know that some part of it was interacted with, and it can report that occurred, but it doesn't necessarily need to know what to do.
Knowing that the liked value needs to be toggled is part of the business logic of the application, and as we have larger applications, we'd like to be able to refactor that sort of code (business logic) out of the react components as much as possible so that they can be tested more easily, and possibly reused across different kinds of projects.
| const App = () => { | ||
| const [messageData, setMessageData] = useState(chatMessages); | ||
| const updateMessageData = (updatedMessage) => { | ||
| const messages = messageData.map((message) => { |
There was a problem hiding this comment.
Nice job merging the updated record into the new list.
If the logic to flip the liked value were up here, instead of injecting the passed object, we would spread the matching object, and update the liked value in the copy.
| }); | ||
| setMessageData(messages); | ||
| }; | ||
| const totalLikes = () => { |
There was a problem hiding this comment.
Nice helper to calculate the number of liked messages.
This would be a great situation to use reduce too!
2 participants
No description provided.