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!
| "dot-notation": "warn", | ||
| "space-before-function-paren": "off", | ||
| "indent": ["warn", 2], | ||
| "indent": ["warn", 'tab'], |
There was a problem hiding this comment.
What led you to change this? There are definitely arguments for using tabs rather than spaces, but in that case, what tab width are you using locally? GitHub assumes 8 spaces (!) for tabs, which would be deep even for Python, but is especially cumbersome in JS, where there is a lot of indentation due to nested anonymous functions.
| <header> | ||
| <h1>Chat between {entries[0].sender} and {entries[1].sender}</h1> | ||
| </header> | ||
| <section id="heartWidget"className="widget">{totalLikes} ❤️{totalLikes !== 1 ? '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 would avoid validation warnings.
There was a problem hiding this comment.
👍 Nice logic to handle pluralization.
| "test": "vitest --run" | ||
| }, | ||
| "dependencies": { | ||
| "date-fns": "^4.1.0", |
There was a problem hiding this comment.
No need to bring in another dependency.
| <h2 className="entry-name">{props.sender}</h2> | ||
| <section className="entry-bubble"> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time"><TimeStamp time={props.timeStamp} /></p> |
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!
| ChatEntry.defaultProps = { // added dafault props so it stays always defined | ||
| onToggleLike: () => {}, | ||
| }; |
There was a problem hiding this comment.
React reports a warning about defaultProps being slated for removal, and to prefer to use JS native default paramaters instead (which can work with destructured object arguments).
| const updatedEntries = entries.map(entry => { | ||
| if (entry.id === id) { | ||
| return { ...entry, liked: !entry.liked } ; | ||
| } else{ | ||
| 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 totalLikes = 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.
| import PropTypes from 'prop-types'; | ||
| import ChatEntry from './ChatEntry'; | ||
|
|
||
| const ChatLog = ({ entries, onToggleLike }) => { |
There was a problem hiding this comment.
ChatEntry uses a single props param, but ChatLog uses a destructured object. Personally, I prefer the destructured style, since it makes the expected component attributes a bit more clear. And it's fine to use a mixture of styles in this project, but in general, try to pick one style or the other.
| const localSender = props.sender === 'Vladimir'; | ||
| const entryClass = `chat-entry ${localSender ? 'local' : 'remote'}`; |
There was a problem hiding this comment.
Nice logic to decide whether to treat a message as local or remote. How could we generalize this so that it didn't need to look explicitly for Vladimir? This project only passes in a single conversation, but ideally, our components should work in other situations.
In the general case, does the ChatEntry itself have enough information as it is to "know" which messages are local and which are remote?
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Chat between {entries[0].sender} and {entries[1].sender}</h1> |
There was a problem hiding this comment.
Nice way to read the participant names from the messages. Notice that this assumes there are only two participants in this conversation, and that they are found in the first two messages. What would happen for other conversation situations?
- no one has sent a message yet
- only one participant has sent a message
- a single participant sends multiple messages before getting a response
- there are more than two participants in this conversation
Some of these cases might not really be workable given the limited data we're working with, but it's worth thinking about what this could look like for a more complete application.
2 participants
No description provided.