-
Notifications
You must be signed in to change notification settings - Fork 45
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
Nikki Vaughan Sphinx #45
base: main
Are you sure you want to change the base?
Conversation
…ges. Add optional enhancements for title, and local vs remote messages.
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.
Nice work! Your project lets me know that you have a goo understanding of React and useState
!
|
||
const App = () => { | ||
const [messages, setMessages] = useState(messageData); | ||
const [likedCount, setLikedCount] = useState(0); |
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.
Do we necessarily need this piece of state? Couldn't we just use the fact that if a message is liked then we could have a function that calculates the total number of likes by iterating over the messages that is chatData
? This works each render since each time a message is liked chatData
state is changed.
const toggleLikedMessage = (messageId) => { | ||
const updatedMessages = messages.map((message) => { | ||
return message.id === messageId | ||
? { ...message, liked: !message.liked } | ||
: message; | ||
}); |
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.
Nice work on this function! By passing this down to your individual chat messages you are now able to have a single source of truth!
|
||
const likedList = updatedMessages.filter((message) => message.liked); | ||
|
||
setLikedCount(likedList.length); |
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.
Like mentioned previously, we could ditch the extra piece of state and do the following:
const totalLikes = chatMessages.reduce((count, message) => count + (message.liked ? 1 : 0), 0);
Then we could use it like so:
<p className='widget' id='heartWidget'>{totalLikes} ❤️s</p>
const participantList = []; | ||
messages.forEach((message) => { | ||
if (!participantList.includes(message.sender)) { | ||
participantList.push(message.sender); | ||
} | ||
}); | ||
|
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.
Nice this would handle multiple (2+) participants!
<h1>Chat between </h1>{' '} | ||
<h1 className={localColor}>{participantList[0]}</h1>{' '} | ||
<h1>and</h1>{' '} | ||
<h1 className={remoteColor}>{participantList[1]}</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.
Perhaps we could combine some of the tags here to make it more readable. Especially since we are using a lot of the same tags.
<ChatLog | ||
entries={messages} | ||
onMessageLiked={toggleLikedMessage} | ||
localColor={localColor} | ||
remoteColor={remoteColor} | ||
/> |
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.
⭐️
entries: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
id: PropTypes.number.isRequired, | ||
sender: PropTypes.string.isRequired, | ||
body: PropTypes.string.isRequired, | ||
timeStamp: PropTypes.string.isRequired, | ||
liked: PropTypes.bool.isRequired, | ||
})).isRequired, | ||
onMessageLiked: PropTypes.func.isRequired, | ||
localColor: PropTypes.string, | ||
remoteColor: PropTypes.string, |
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.
⭐️
const isRemote = props.sender === 'Estragon' ? true : false; | ||
const senderLocation = isRemote | ||
? 'chat-entry remote' | ||
: 'chat-entry local'; | ||
const senderColor = isRemote | ||
? props.remoteColor | ||
: props.localColor; |
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.
Ternary Terminal lol!
onClick={ | ||
() => props.onMessageLiked(props.id) | ||
}>{props.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.
Nice work!
id: PropTypes.number.isRequired, | ||
sender: PropTypes.string.isRequired, | ||
body: PropTypes.string.isRequired, | ||
timeStamp: PropTypes.string.isRequired, | ||
liked: PropTypes.bool.isRequired, | ||
onMessageLiked: PropTypes.func.isRequired, | ||
localColor: PropTypes.string, | ||
remoteColor: PropTypes.string, |
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.
⭐️
No description provided.