Conversation
kelsey-steven-ada
left a comment
kelsey-steven-ada
left a comment
There was a problem hiding this comment.
Great work! Let me know if you have any questions on the feedback below =]
| ); | ||
| }; | ||
|
|
||
| const totalLikes = entries.reduce((acc, entry) => acc + Number(entry.liked), 0); |
There was a problem hiding this comment.
Great use of the existing data and array.reduce to calculate the total likes!
| const handleLike = (id) => { | ||
| setEntries((prevEntries) => | ||
| prevEntries.map((entry) => | ||
| entry.id === id ? { ...entry, liked: !entry.liked } : entry | ||
| ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Nice work ensuring we're creating a new array of messages to trigger the re-render after updating the liked value!
| const handleLikeClick = | ||
| typeof onHandleLike === 'function' | ||
| ? () => onHandleLike(message.id) | ||
| : undefined; |
There was a problem hiding this comment.
It is generally considered against best practices to create a function we will be passing to children components inside of the mapping function itself.
Thinking about the single responsibility principle: we are doing two things at once by both defining functionality for a button press and mapping over entries to create a list of components.
Looking at the principles of encapsulation and packaging related code in the same file: the functionality for handleLikeClick seems tied to the ChatEntry component. ChatLog defines this function but never uses it itself, it only references the function to pass it to ChatEntry.
How can we refactor this code so that we are not defining functionality inside of a mapping function and so that the code lives with the component that will use it?
| const handleLike = (id) => { | ||
| setEntries((prevEntries) => | ||
| prevEntries.map((entry) => | ||
| entry.id === id ? { ...entry, liked: !entry.liked } : entry |
There was a problem hiding this comment.
I like this pattern of sending just the id to handleLike since it keeps all the state management and message object creation confined to App.
| <p className="entry-time"> | ||
| <TimeStamp time={timeStamp} /> | ||
| </p> |
There was a problem hiding this comment.
Great thought to wrap this in a p element since TimeStamp returns a span as the outer element!
| <button className="like" onClick={onHandleLike}> | ||
| {heart} | ||
| </button> |
There was a problem hiding this comment.
We can increase the accessibility of our page by setting further attributes on elements, especially interactive elements. Below is an example of updates we could make to the button, check out the MDN docs for more info!
<button
onClick={onHandleLike}
className="like"
aria-label={heart}
role="img"
>
{heart}
</button>| <article className="chat-entry local"> | ||
| <h2 className="entry-name">{sender}</h2> |
There was a problem hiding this comment.
If we wanted to apply the optional enhancements to align the chat bubbles, we could use a ternary operator to decide the remote or local className based on the sender. Then we could use an interpolated string to create the className which always holds chat-entry along with a placeholder where we pass the variable holding the remote or local class name.
| : undefined; | ||
|
|
||
| return ( | ||
| <li key={message.id}> |
There was a problem hiding this comment.
Great use of the unique message ids for the key value!
|
|
||
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf(PropTypes.shape({ |
There was a problem hiding this comment.
Great use of Prop-Types and flagging those necessary for render with isRequired, I love the use of PropTypes.shape to ensure the passed array has the necessary contents as well!
2 participants
No description provided.