C22 -Phoenix -- Beenish Ali #35
Conversation
| import PropTypes from 'prop-types'; | ||
| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = ({sender, body, timeStamp, id, liked, onLiked}) => { |
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.
| <section className="entry-bubble"> | ||
| <p>{body}</p> | ||
| <p className="entry-time"> | ||
| <TimeStamp time = {timeStamp} /> |
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!
Nit: no spaces around attribute =
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| id: PropTypes.number.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| onLiked: 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.
| const chatEntryComponents = entries.map((chatEntry) => { | ||
| return ( | ||
| <ChatEntry | ||
| key={chatEntry.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).
| import PropTypes from 'prop-types'; | ||
|
|
||
| const ChatLog = ({entries, onLiked}) => { | ||
| const chatEntryComponents = entries.map((chatEntry) => { |
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 can make debugging a little more tricky. But any of those approaches will work fine.
| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = ({sender, body, timeStamp, id, liked, onLiked}) => { | ||
| const heart = 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.
| </p> | ||
| <button | ||
| className="like" | ||
| onClick={onLikeClicked}> |
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.
| const ChatEntry = ({sender, body, timeStamp, id, liked, onLiked}) => { | ||
| const heart = liked ? '❤️' :'🤍' ; | ||
|
|
||
| const localOrRemote = sender === 'Vladimir' ? 'chat-entry local' : 'chat-entry 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?
| function App () { | ||
| const [messageData, setMessageData] = useState(Data); | ||
|
|
||
| const senders = [...new Set(Data.map(message => message.sender))]; |
There was a problem hiding this comment.
Nice use of map to extract the sender information, then uniquing that with a Set to get the participants in the conversation. We can confirm with the docs that iterating over the Set contents will occur in insertion order, meaning the resulting list of senders, however many there are, will be in the order they participated in the conversation.
| <> | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Chat between {senders[0]} and {senders[1]}</h1> |
There was a problem hiding this comment.
For the conversation in the project, there are exactly two participants and both have sent messages, which is the only reason we can find them. What would happen for other conversation situations?
- no one has sent a message yet
- only one participant has sent a message (the local user? a remote user?)
- 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.