ChatLog - Luqi Xie C22 Pheonix #29
Conversation
| }, | ||
| "dependencies": { | ||
| "luxon": "^2.5.2", | ||
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1" | ||
| "react-dom": "^18.3.1", | ||
| "predeploy": "npm run build", |
There was a problem hiding this comment.
👀 This line "predeploy" is intended to define a script, but is listed in the dependencies section. This prevented npm from being able to install the project. This should be moved up to the "scripts" section.
| import TimeStamp from './TimeStamp'; | ||
|
|
||
|
|
||
| const ChatEntry = ({ id, sender, body, timeStamp, liked, onLike }) => { |
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) I really prefer the glanceability.
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p>{body}</p> | ||
| <p className="entry-time"><TimeStamp time={timeStamp}></TimeStamp></p> |
There was a problem hiding this comment.
Nice use of the supplied TimeStamp. All we need to do is 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!
Note that the TimeStamp component doesn't receive children components (it resembles "void" HTML tags such as input or img rather than tags like p or a that can have children). For such components, prefer to write them using "self-closing" style (note the /> to complete the tag rather than a regular >).
<TimeStamp time={timeStamp} />| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| onLike: PropTypes.func, |
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, as you did.
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.
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.
There was a problem hiding this comment.
It should be possible to add dist to the .gitignore so the dist folder doesn't get checked in to your code branch. Build products generally shouldn't get checked into the source repo.
| const toggleLike = (id) => { | ||
| setChatData(chatData => chatData.map(chat => { | ||
| if (chat.id === id) { | ||
| return {...chat, liked: !chat.liked }; |
There was a problem hiding this comment.
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.
| const calculateTotalLikes = (chatData) => { | ||
| return chatData.reduce((total, chat) => { | ||
| return total + chat.liked; | ||
| }, 0); | ||
| }; | ||
|
|
||
| const totalLikes = calculateTotalLikes(chatData); |
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.
I like that you wrapped your reduce call in a well-named helper function. This is one way that we can make using reduce a little more understandable for other programmers reading our code, since the syntax can be a little confusing. Another way to make this self-describing is to give a clear name to the function we pass to reduce.
const sumLikes = (total, chat) => {
return chat.liked ? total + 1 : total;
};
const totalLikes = chatData.reduce(sumLikes, 0);|
|
||
| const calculateTotalLikes = (chatData) => { | ||
| return chatData.reduce((total, chat) => { | ||
| return total + chat.liked; |
There was a problem hiding this comment.
This works as is, since liked is a boolean value, and JS will coerce true to 1 and false to 0. Still, it can be surprising to see this. Consider using the liked boolean value to determine whether to increment the count
return chat.liked ? total + 1 : total;or to explicitly pick the value to add
return total + (chat.liked ? 1 : 0)|
|
||
|
|
||
| const ChatEntry = ({ id, sender, body, timeStamp, liked, onLike }) => { | ||
| const heartColor = liked ? '❤️' : '🤍'; |
There was a problem hiding this comment.
👍 We can figure out which emoji to use for the liked status based on the liked prop without creating any additional state.
| <button className="like">🤍</button> | ||
| <p>{body}</p> | ||
| <p className="entry-time"><TimeStamp time={timeStamp}></TimeStamp></p> | ||
| <button className="like" onClick={() => onLike(id)}>{heartColor}</button> |
There was a problem hiding this comment.
👍 We need a wrapper of some kind rather than calling the received callback through props, since our callback function is expecting a message id as its parameter. If we tried to use it directly as the click event handler, React would end up passing it a clink event, since any function registered as an event handler will always be given the event detail information as its argument.
No description provided.