Conversation
…s a getColor function
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Looks good 🎉 I've left some feedback as comments, please check them out when you can and reach out here or on Slack if there's anything I can clarify =]
| const [localColor, setLocalColor] = useState(''); | ||
| const [remoteColor, setRemoteColor] = useState(''); |
There was a problem hiding this comment.
If both local and remote start out with black as the default color, that could be the initial value for these state variables.
| msg.body, | ||
| msg.timeStamp, | ||
| msg.liked, | ||
| isLocal |
There was a problem hiding this comment.
Neat way to add the local/remote data to the messages. In this case where we only have 2 senders, I might suggest using the value of sender to determine remote vs local over holding a new piece of data, but this approach will scale better.
| const [chatEntries, setChatEntries] = useState(messages); | ||
| const [localColor, setLocalColor] = useState(''); | ||
| const [remoteColor, setRemoteColor] = useState(''); | ||
| const [totalLikes, setTotalLikes] = useState(initialLikes); |
There was a problem hiding this comment.
Since the liked status of a message lives in the chatEntries data we should avoid holding an extra piece of state that we need to manually keep in sync. We can use a higher order function like array.reduce to take our list of messages and reduce it down to a single value.
// This could be returned from a helper function
// totalLikes is a variable that accumulates a value as we loop over each entry in chatEntries
const likesCount = chatEntries.reduce((totalLikes, currentMessage) => {
// If currentMessage.liked is true add 1 to totalLikes, else add 0
return (totalLikes += currentMessage.liked ? 1 : 0);
}, 0); // The 0 here sets the initial value of totalLikes to 0| <h1>Application title</h1> | ||
| <h1> | ||
| Chat Between{' '} | ||
| <span className={localColor}>{chatEntries[0].sender}</span> and{' '} |
There was a problem hiding this comment.
Nice use of new lines to keep the contents of the h1 clear.
| const clickLike = (e) => { | ||
| onUpdateEntry( | ||
| new ChatMessage( | ||
| message.id, | ||
| message.sender, | ||
| message.body, | ||
| message.timeStamp, | ||
| !message.liked, | ||
| message.isLocal | ||
| ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
I would consider passing the id of the message clicked to onUpdateEntry and having the App code handle the new message creation. When ChatEntry creates the new object for the App state, it takes some responsibility for managing those contents. If we want the responsibility of managing the state to live solely with App, we would want it to handle defining the new message object.
This made me think of a related concept in secure design for APIs. Imagine we had an API for creating and updating messages, and it has an endpoint /<msg_id>/like meant to update a true/false liked value. We could have that endpoint accept a body in the request and let the user send an object with data for the message's record (similar to passing a message object from ChatEntry to App), but the user could choose to send any data for those values. If the endpoint only takes in an id and handles updating the liked status for the message itself, there is less opportunity for user error or malicious action.
There was a problem hiding this comment.
Thank you! I understand!
| ); | ||
| }; | ||
|
|
||
| ColorChoice.propTypes = { |
There was a problem hiding this comment.
Great use of PropTypes across the files =]
2 participants
My test aren't going to pass because I use an object to keep track of each message. I also used a prop function to get the correct color for each message according if they were local or not.