-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ocelots - Eva Liu #139
base: main
Are you sure you want to change the base?
Ocelots - Eva Liu #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work 🎉 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 =]
setChatsData(chats); | ||
}; | ||
|
||
const heartCounts = chatsData.filter((chat) => chat.liked === true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work calculating the hearts count from the chatsData! Since we don't need the contents of the array we get from filter, another option is to use a higher order function like array.reduce
to take our list of messages and reduce it down to a single value.
// totalLikes is a variable that accumulates a value as we loop over each entry in chatEntries
const heartCounts = chatsData.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
const changeLikeData = () => { | ||
const chatData = { | ||
id: props.id, | ||
sender: props.sender, | ||
body: props.body, | ||
timeStamp: props.timeStamp, | ||
liked: !props.liked, | ||
}; | ||
props.updateChatsData(chatData); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would consider passing the id
of the message clicked to props.updateChatsData
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 /<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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Kesley,
regarding the secure design for APIs topic, will that also apply to the PUT and PATCH routes? I was trying on these two when I was building up the like counts for the inspiration board, and I realized that the PATCH from the front-end needed to add the request body, but PUT didn't. I tried and both worked as the expected result.
And I read another answer from the stack Overflow (not sure if it's correct), the PUT will update all data and the PATCH will only update the one that has been selected. I wasn't sure which way is better approach.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does also apply to PATCH where we're typically only updating one or some of the values. In that case we have to send some data for the values that are changing, but we should choose to only let folks send the relevant data, rather than data for the entire object. When it comes to PUT, we're generally replacing an entire resource so we need to send all the data for the object. In public applications we want to do a lot of data sanitization when taking in input from users to ensure that it is only the types that are allowed and that we escape special characters which can help prevent running malicious code sent in place of text.
}; | ||
|
||
ChatLog.propTypes = { | ||
entries: PropTypes.arrayOf( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice use of PropTypes.
No description provided.