-
Notifications
You must be signed in to change notification settings - Fork 17
Monica --- Ocelots #6
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
base: main
Are you sure you want to change the base?
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.
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 [numberLikes, setNumberLikes] = useState(0); | ||
|
|
||
| const updateChatEntry = (chatToUpdate) => { | ||
| const chats = chatEntries.map((entry) => { | ||
| if(entry.id === chatToUpdate.id){ | ||
| setNumberLikes(chatToUpdate.liked ? numberLikes+1 : numberLikes-1) | ||
| return chatToUpdate; | ||
| } | ||
| return entry; | ||
| }); | ||
| setChatEntries(chats); | ||
| }; |
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.
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 (our like count).
// 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| <header> | ||
| <h1>Application title</h1> | ||
| <div className='applicationTitle'> | ||
| <h1>Chat between <span className='green'>Vladimir</span> and <span className='blue'>Estragon</span> </h1> |
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 is a little bit of personal taste, but I suggest dropping the spans to new, indented lines to make it easier to see at a glance what is nested inside the h1.
| const updateClickState = () => { | ||
| onUpdateChat({ | ||
| id, | ||
| sender, | ||
| body, | ||
| timeStamp, | ||
| liked : !liked | ||
| }) | ||
| } |
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 onUpdateChat 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.
| if(sender === 'Vladimir'){ | ||
| entryClass = 'local'; | ||
| }else{ | ||
| entryClass = 'remote'; | ||
| } |
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 is a great place for a javascript ternary operator:
const entryClass = (sender === 'Vladimir') ? 'local' : 'remote';| const ChatLog = ({entries, onUpdateChat}) => { | ||
| const messages = entries.map((message, index) => { | ||
| return( | ||
| <section key = {index}> |
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.
If the messages all have unique ids, then we could use those values for the key over generating and using indices.
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf( | ||
| PropTypes.shape({ |
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.