Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good! Please review my comments, and let me know if you have any questions. Nice job!
There was a problem hiding this comment.
Was there a particular issue you ran into that needed a version update?
| onLikeClick: PropTypes.func.isRequired, | ||
| isLocal: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
The id (omitted here), sender, body, timeStamp, and liked props are always passed (they're defined explicitly in the data and also provided in the test) so we can (and should) mark them isRequired.
The remaining props were up to you, and the tests don't know about them. As a result, using isRequired causes a warning when running any tests that only pass the known props. If you didn't see those warnings when running the tests, be sure to also try running the terminal npm test since the warnings are more visible.
To properly mark any other props isRequired, we would also need to update the tests to include at least dummy values (such as an empty callback () => {} for the like handler) to make the proptype checking happy.
Alternatively, for any props that we leave not required, we should also have logic in our component to not try to use the value if it's undefined.
| timeStamp={entry.timeStamp} | ||
| liked={entry.liked} | ||
| isLocal={entry.sender === localSender} | ||
| onLikeClick={() => onLikeToggle(entry.id)} |
There was a problem hiding this comment.
🔎 In general, defining the callback wrapper in the parent component while iterating over the data solves the same problem as passing the callback and id into the child component and creating the event wrapper there. It even allows the id to be omitted (the child component calls the supplied function without needing to supply any parameters, since they are backed into the individualized callback it received).
However, we should note that in the tests for ChatEntry, the id was supplied to the child component, implying that we are expecting it to be received and used by ChatEntry. Therefore, in this case, we should receive the message id as one of the props, and create the wrapper for the event handler here in the ChatEntry. This also mirrors the approaches we took for creating event handlers during class.
| <section className="widget" id="heartWidget"> | ||
| {likedCount} ❤️s | ||
| </section> |
There was a problem hiding this comment.
section tags are recommended to have a heading within them. Use of an h2 around the liked count (with appropriate className and id attached to pick up the styles in App.css) would avoid validation warnings, as well as resemble the sample site more closely.
| <button className="like">🤍</button> | ||
| <p>{body}</p> | ||
| <p className="entry-time"> | ||
| <TimeStamp time={timeStamp} /> |
There was a problem hiding this comment.
Nice use of the supplied TimeStamp. We pass in the timeStamp string from the message data and it takes care of the rest. All we had to do was confirm the name and type of the prop it was expecting (which we could do through its PropTypes) and we're all set!
| liked: PropTypes.bool.isRequired, | ||
| }) | ||
| ).isRequired, | ||
| onLikeToggle: PropTypes.func.isRequired, |
There was a problem hiding this comment.
Similar to the props for ChatEntry here, the entries prop is included in the tests, but the like toggle is not, resulting in prop warnings (unless we update the tests to reflect our custom props).
Again, if we were to leave this as not required so as to avoid the test warnings, we'd want to be sure that all the script logic in our component worked properly even in the absence of this value.
| <TimeStamp time={timeStamp} /> | ||
| </p> | ||
| <button className="like" onClick={onLikeClick}> | ||
| {liked ? '❤️' : '🤍'} |
There was a problem hiding this comment.
👍 Nice use of a ternary to pick the emoji to show for the liked status. Consider moving even something small like this to a helper function external to the component, so that the behavior of the emoji-picking logic can be tested easily outside of React.
| const updatedEntries = entries.map((entry) => { | ||
| if (entry.id === id) { | ||
| return { ...entry, liked: !entry.liked }; | ||
| } | ||
| return entry; | ||
| }); | ||
| setEntries(updatedEntries); |
There was a problem hiding this comment.
In this case, calculating the next version of the message data using the current state variable and passing that updated version to the state setter shouldn't cause any problems, but we still generally prefer using the callback style of setters. Using that approach, we pass a function to the setter whose parameter will receive the latest state value, and which returns the new value to use for the state.
setEntries(entries => entries.map(entry => {
if (entry.id === id) {
return {...entry, liked: !entry.liked};
} else {
return entry;
};
}));We showed this approach in class, but technically, we're mixing a few responsibilities here. rather than this function needing to know how to change the liked status itself, we could move this update logic to a helper function. This would better mirror how we eventually update records when there's an API call involved.
In this project, our messages are very simple objects, but if we had more involved operations, it could be worthwhile to create an actual class with methods to work with them, or at least have a set of dedicated helper functions to centralize any such mutation logic.
| setEntries(updatedEntries); | ||
| }; | ||
|
|
||
| const likedCount = entries.filter(entry => entry.liked).length; |
There was a problem hiding this comment.
Nice job determining the total likes based on the like data of each message. We don't need an additional piece of state to track this, since it can be derived from the existing state we are tracking.
Great idea to filter down the data to the liked messages and use the list length as the count. This is a really understandable alternative to the more canonical reduce approach. Compared to reduce, it does make a list potentially as long as the message list, but since we know a similar copying gets done anyway during a render (building the ChatEntry components) on the whole it's not any more costly.
|
|
||
| const ChatEntry = () => { | ||
|
|
||
| const ChatEntry = ({ sender, body, timeStamp, liked, onLikeClick, isLocal }) => { |
There was a problem hiding this comment.
👍 I like using destructured props to make it more visibly clear in the function definition itself what props we're expecting to receive. We do need to remember that these are all passed in as a single object (the one we usually call props) and it can cause problems if we forget to include the destructuring syntax (it's easy to forget and list the props as multiple separate params), but I really prefer the glanceability.
2 participants
No description provided.