AC2-Ocelots-Elaine(Si Ok) Sohn #4
Conversation
|
|
||
| const App = () => { | ||
| const [chatMessages, setChatMessages] = useState(MESSAGES); | ||
| const [likesCount, setLikesCount] = useState(initialLiked); |
There was a problem hiding this comment.
Since the liked status of a message lives in the chatMessages data we should avoid holding an extra piece of state that we need to manually keep in sync. We can remove likesCount declared on line 25 and 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 = chatMessages.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| // Arrange | ||
| const { container } = render(<App />) | ||
| const buttons = container.querySelectorAll('button.like') | ||
| console.log(buttons) |
There was a problem hiding this comment.
Gentle reminder that we want to take a scan through our files and remove diagnostic/debugging code before opening PRs.
| const toggleHeart = () => { | ||
| return onUpdate({ | ||
| id, | ||
| sender, | ||
| body, | ||
| timeStamp, | ||
| liked: !liked, | ||
| }) | ||
| }; |
There was a problem hiding this comment.
I would consider passing the id of the message clicked to 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 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.
| (entry.sender === localSender)? | ||
| <ChatEntry | ||
| // key={entry.id} | ||
| key={entry.timeStamp} | ||
| id={entry.id} | ||
| sender={entry.sender} | ||
| body={entry.body} | ||
| timeStamp={entry.timeStamp} | ||
| liked={entry.liked} | ||
| onUpdate = {onUpdate} | ||
| color={localColor} | ||
| location='local' | ||
| /> : | ||
| <ChatEntry | ||
| // key={entry.id} | ||
| key={entry.timeStamp} | ||
| id={entry.id} | ||
| sender={entry.sender} | ||
| body={entry.body} | ||
| timeStamp={entry.timeStamp} | ||
| liked={entry.liked} | ||
| onUpdate = {onUpdate} | ||
| color={remoteColor} | ||
| location='remote' | ||
| /> |
There was a problem hiding this comment.
There's a lot of duplication between the 2 ChatEntry components, we could create a couple variables above where we create the ChatEntry to determine the color and location values which would let us keep just one of the ChatEntry declarations.
let color = remoteColor
let location = 'remote'
if (entry.sender === localSender) {
color = localColor
location = 'local'
}
<ChatEntry
// key={entry.id}
key={entry.timeStamp}
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
onUpdate = {onUpdate}
color={color}
location={location}
/>|
|
||
| const ChatLog = ({entries, onUpdate, localColor, remoteColor, localSender}) => { | ||
| return <ul className="chat-log"> | ||
| {entries.map((entry) => ( |
There was a problem hiding this comment.
It is a little hard to understand the structure of the JSX with the mapping embedded here, I suggest pulling the mapping out and storing the result of it in a variable that you can then place in the JSX.
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf( | ||
| PropTypes.shape({ |
There was a problem hiding this comment.
Really nice use of PropTypes.
2 participants
No description provided.