Conversation
mikellewade
left a comment
mikellewade
left a comment
There was a problem hiding this comment.
Olya, nice work on your React Chatlog project!
| import ChatLog from './components/ChatLog'; | ||
|
|
||
| const App = () => { | ||
| const [messageList, setMessageList] = useState(messagesData); |
| const likeToggleFunc = (messageId) => { | ||
| const messages = messageList.map(message => { | ||
| if (message.id === messageId) { | ||
| return { ...message, liked: !message.liked }; | ||
| } else { | ||
| return message; | ||
| } | ||
| }); | ||
|
|
||
| setMessageList(messages); | ||
| }; |
There was a problem hiding this comment.
Nice work on using .map to map over your entries! Very functional! Since you are using a simple conditional block here, we could make this more succinct by using a ternary instead like so:
const toggleLike = (id) =>
setEntries(entries.map(entry =>
entry.id === id ? { ...entry, liked: !entry.liked } : entry
));When you find yourself with simple checks like these, more often than not a ternary could be implemented instead for conciseness but still maintaining some kind of readability.
| const totalLikes = messageList | ||
| .filter((message) => message.liked) | ||
| .length; |
There was a problem hiding this comment.
You could also do this with the.reduce method like so:
const totalLikes = chatMessages.reduce((count, message) => {
return count + (message.liked ? 1 : 0)
}, 0);As for the formatting, I think you could leave this code on a single since it isn't longer than 80 characters.
| const totalLikes = messageList | |
| .filter((message) => message.liked) | |
| .length; | |
| const totalLikes = messageList.filter((message) => message.liked).length; |
| <h1>Chat between Vladimir and Estragon</h1> | ||
| <div> | ||
| {totalLikes} ❤️s |
| entries={messageList} | ||
| likeToggleFunc={likeToggleFunc} | ||
| localSender="Vladimir" |
There was a problem hiding this comment.
A couple of things here:
-
You have the data named
messageListbut then name the propentries, is there a reason as to why you chose not to consistently name key in the prop objectmessageList? Though there is nothing technically wrong with what you have, for readability you will want to stick with consistent naming. It will help yourself and other developers keep track of what data is coming from where. -
For your
likeToggleFunc, I would name it something likeonLikeToggleoronLike, this more readily communicates that the function will be used for an event. -
You hard coded the local sender here, but what could be do to make this more dynamic? What would happen here if the messages no longer with Vladimir?
| // Fill with correct proptypes | ||
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| senderType: PropTypes.oneOf(['local', 'remote']).isRequired, |
| const likeButtonClicked = () => { | ||
| props.likeToggleFunc(props.id); | ||
| }; |
| return ( | ||
| <div className="chat-entry local"> | ||
| <h2 className="entry-name">Replace with name of sender</h2> | ||
| <div className={props.senderType === 'local' ? 'chat-entry local' : 'chat-entry remote'}> |
There was a problem hiding this comment.
You could also assign this ternary logic to a variable before your return statement for readability. However, note that you have very similar logic here and in ChatLog.jsx. How could you refactor this logic to DRY up your components? @omatesh
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time">Time Sent: <TimeStamp time={props.timeStamp} /></p> |
| <button className="like" onClick={likeButtonClicked}> | ||
| {props.liked ? '❤️' : '🤍'} |
There was a problem hiding this comment.
The button function could also be implemented like so:
<button className='like' onClick={() => props.likeToggleFunc(props.id)}>{liked ? '❤️': '🤍'}</button>2 participants
No description provided.