C22- Phoenix- Alejandra G. #33
Conversation
| import TimeStamp from './TimeStamp'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const ChatEntry = (props) => { |
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.
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <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. 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!
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| toggleLiked: PropTypes.func.isRequired, |
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) marked 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: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| })).isRequired, | ||
| toggleLiked: 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={entry.timeStamp} | ||
| liked={entry.liked} | ||
| toggleLiked={toggleLiked} | ||
| key={entry.id} |
There was a problem hiding this comment.
👍 The key attribute is important for React to be able to detect certain kinds of data changes in an efficient manner. We're also using the id for our own id prop, so it might feel redundant to pass both, but one is for our logic and one is for React internals (we can't safely access the key value in any meaningful way).
|
|
||
| const App = () => { | ||
| const [chatData, setChatData] = useState(chatMessages); | ||
| const toggleLiked = (id) => { |
There was a problem hiding this comment.
👍 Since our state is defined here, we also need to define our mutating function here so that it can "see" the setter function. All we need to receive is the id of the message to toggle, which allows us to locate the message to update, and calculate the next state value. We can pass this all the way down to our ChatEntry which handles the click event, passing us the id of the message that was clicked.
|
|
||
| const ChatEntry = (props) => { | ||
| const toggleLikeButton = () => { | ||
| props.toggleLiked(props.id); |
There was a problem hiding this comment.
👍 Passing the id of this message lets the logic defined up in the App find the message to update in its data.
| props.toggleLiked(props.id); | ||
| }; | ||
|
|
||
| const heartButton= props.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>{props.body}</p> | ||
| <p className="entry-time"><TimeStamp time={props.timeStamp} /></p> | ||
| <button onClick={toggleLikeButton} className="like">{heartButton}</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.
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| <h1>Chat between {chatData[0].sender} and {chatData[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.
No description provided.