Skip to content

Conversation

@CatherineBandarchuk
Copy link

Did not implement optional wave4 functionality completely.

Copy link

@kelsey-steven-ada kelsey-steven-ada left a 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 =]

Comment on lines +20 to +28
const calculateLikes = (entries) => {
let total = 0;
for (const entry of entries) {
if (entry.liked) {
total++;
}
}
return total;
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great use of the existing message data to derive the number of liked messages! Another option could be to 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 chatEntryData
const likesCount = chatEntryData.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

Comment on lines +7 to +16
const updateLikes = () => {
const updateEntry = {
id: props.id,
sender: props.sender,
body: props.body,
timeStamp: props.timeStamp,
liked: !props.liked,
};
props.onUpdate(updateEntry);
};

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.onUpdate 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 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.

return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div className={bubbleClass}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another option could 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 bubbleClass = (props.sender === 'Vladimir') ? 'local' : 'remote';
...
<div className={`chat-entry ${bubbleClass}`}>

};

ChatLog.propTypes = {
messages: PropTypes.arrayOf(

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants