Conversation
anselrognlie
left a comment
anselrognlie
left a comment
There was a problem hiding this comment.
Looks good overall, but please review my comments, and let me know if you have any questions.
| <main> | ||
| {/* Wave 01: Render one ChatEntry component | ||
| Wave 02: Render ChatLog component */} | ||
| <h2 className="like-count">{likedCount} ❤️s</h2> |
There was a problem hiding this comment.
Consider moving this up to be a part of the header. As positioned, there's a large padding above this element, and the likes scrolls away as we move through the conversation.
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p className='message-body'>{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!
| onLikeCountChange: PropTypes.func, //remove isRequired to prevent test error | ||
| isLocal: PropTypes.bool, |
There was a problem hiding this comment.
The id, 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 would cause a warning when running any tests that only pass the known props. As a result, either leaving the prop not marked isRequired (as you did) or updating the tests to pass a dummy function is required to avoid the warnings.
If we do leave props marked not isRequired, we should either have logic to avoid using those values if they are missing, or make allowance for using a default value. As written, we could leave off the onLikeCountChange with no warning, but then have the code crash when we try clicking the like button.
| entries: PropTypes.arrayOf(PropTypes.shape({ | ||
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| })), | ||
| onLikeCountChange: PropTypes.func, //remove isRequired to prevent test error | ||
| local: PropTypes.string, |
There was a problem hiding this comment.
Similar to the props for ChatEntry, here, we have some props that the tests pass and some they don't. The ones that the tests pass (entries) can be marked required without issue, while marking anything else required would cause warnings. Alternatively, we could update the tests to pass these additional props once we have defined them.
We should remember that if we leave props not required, we should include appropriate logic to check for a value before making use of it (e.g., we shouldn't try to call an optional callback without ensuring we have a function to call!).
| import PropTypes from 'prop-types'; | ||
|
|
||
| const ChatLog = (props) => { | ||
| const chatEntryComponents = props.entries.map((chat, index) => { |
There was a problem hiding this comment.
Nice use of map to convert from the message data into ChatEntry components. We can perform this mapping storing the result into a variable we use in the JSX result as you did here (components are functions, so we can run JS code as usual before we reach the return, and even sometimes have multiple return statements with different JSX), we could make a helper function that we call as part of the return, or this expression itself could be part of the return JSX, which I often like since it helps me see the overall structure of the component, though it can make debugging a little more tricky. But any of those approaches will work fine.
| const chats = chatData.map(chat => { | ||
| if (chat.id === chatId) { | ||
| return { ...chat, liked: !chat.liked }; | ||
| } else { | ||
| return chat; | ||
| } | ||
| }); | ||
|
|
||
| setChatData(chats); |
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.
setChatData(chatData => chatData.map(chat => {
if (chat.id === id) {
return {...chat, liked: !chat.liked};
} else {
return chat;
};
}));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.
| setChatData(chats); | ||
| }; | ||
|
|
||
| const likedCount = chatData.filter((chat) => chat.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 TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = (props) => { |
There was a problem hiding this comment.
Personally, I like using destructured props to make it more visibly clear in the function definition itself what props we're expecting to receive. Even though we do need to remember that these are all passed in as a single object (your props parameter) 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), I really prefer the glanceability.
If you changed your approach to use destructuring here, you'd most likely want to be consistent across all your components.
| import { useState } from 'react'; | ||
|
|
||
| const App = () => { | ||
| const local = messages[0].sender; //I assume the first sender in messages dataset is the local sender |
There was a problem hiding this comment.
👍 We didn't receive any explicit data indicating which of the senders is local and which is remote, so deciding to treat the first sender as local is a reasonable decision.
Prefer to drive this off the state chatData rather than the initial JSON data.
Also, consider what would happen if in this conversation:
- 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.
| for (const message of messages) { | ||
| if (message.sender !== local) { | ||
| remote = message.sender; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
👍 Nice way to pick the first non-local participant as the remote. But be sure to think through some of the cases I mentioned for the makeup of the conversation and consider how this approach to calculating the (assumed) two participants would respond.
2 participants
No description provided.