Olive - React Chatlog - Sea Turtles #95
Conversation
| const getLikeCount = () => { | ||
| let likeCount = 0; | ||
| for (const entry of chatEntries) { | ||
| if (entry.liked === true) { | ||
| likeCount += 1; | ||
| } | ||
| } | ||
| return likeCount; | ||
| }; |
There was a problem hiding this comment.
Great use of the existing data to calculate the total likes! Another option would be to use the array function reduce:
return messageData.reduce((totalLikes, message) => {
// If messages.liked is true add 1 to totalLikes, else add 0
return (totalLikes += message.liked ? 1 : 0);
}, 0); // The 0 here sets the initial value of totalLikes to 0| const updateChatEntry = (updatedEntry) => { | ||
| const entries = chatEntries.map((entry) => { | ||
| if (entry.id === updatedEntry.id) { | ||
| return updatedEntry; |
There was a problem hiding this comment.
Nice managing of data & making sure we return a new object when we need to alter a message in our list.
| const participants = new Set(); | ||
| for (const entry of chatEntries) { | ||
| participants.add(entry.sender); | ||
| } | ||
| return [...participants].join(' and '); |
There was a problem hiding this comment.
Great use of Set to get the unique senders! If we weren't significantly concerned about space, we could consider mapping chatEntries to an array of only the sender values, then use a Set to remove duplicates:
const allNames = chatEntries.map(message => message.sender);
const participants = [...new Set(allNames)];
return participants.join(' and ');| const onLikeButtonClick = () => { | ||
| const updatedChatEntry = { | ||
| id: props.id, | ||
| sender: props.sender, | ||
| body: props.body, | ||
| timeStamp: props.timeStamp, | ||
| liked: !props.liked, | ||
| }; | ||
| props.onLike(updatedChatEntry); |
There was a problem hiding this comment.
I would consider passing the id of the message clicked to props.onLike and having the App code handle the new object 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 /liked/<id> meant to update the 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.
| const displaySide = | ||
| props.sender === 'Vladimir' ? 'chat-entry local' : 'chat-entry remote'; |
There was a problem hiding this comment.
Nice ternary operators & splitting up lines!
| return ( | ||
| <div className="chat-entry local"> | ||
| <h2 className="entry-name">Replace with name of sender</h2> | ||
| <div className={displaySide}> |
There was a problem hiding this comment.
Another option would be to have an interpolated string here that always holds chat-entry and use a placeholder where we pass only the remote or local class name (so we don't repeat chat-entry anywhere):
const displaySide = (props.sender === 'Vladimir') ? 'local' : 'remote';
...
<div className={`chat-entry ${displaySide}`}>| // id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| // liked: PropTypes.bool.isRequired, |
There was a problem hiding this comment.
Nice use of PropTypes and isRequired! Especially when working with other folks, I would recommend keeping all the props we can expect to give to a component listed & uncommented, and only use isRequired on the props that MUST be passed for the component to render (or our tests to pass 😆).
| }; | ||
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf(PropTypes.object).isRequired, |
There was a problem hiding this comment.
We can be even more specific with our PropTypes for validation. If we know that we need an array, and that array's objects need to hold certain data, we can check the objects as well! Inside arrayOf we can pass PropTypes.shape() with information about the object's contents. A modified example from stack overflow is below:
list: PropTypes.arrayOf(
PropTypes.shape({
id: PropTypes.number.isRequired,
customTitle: PropTypes.string.isRequired,
btnStyle:PropTypes.object,
})
).isRequired,Source: https://stackoverflow.com/questions/59038307/reactjs-proptypes-validation-for-array-of-objects
No description provided.