-
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
Sphinx - Leidy Martinez C22 #38
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.
Nice work! Your project lets me know that you have a goo understanding of React and useState
!
const calculateTotalLikeCount = (chat) => { | ||
return chat.reduce((total, chat) => { | ||
return total + (chat.liked ? 1 : 0); | ||
}, 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.
Excellent execution!
const localSender = messages[0].sender; | ||
let remoteSender = 'Unknown'; | ||
|
||
for (const message of messages) { | ||
if (message.sender !== localSender) { | ||
remoteSender = message.sender; | ||
break; // Stop once other sender is found | ||
} | ||
} |
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 handles cases of double texting!
const handleLikeButtom = (id) => { | ||
setChat(chat => { | ||
return chat.map(chat => { | ||
if (chat.id === id) { | ||
return { ...chat, liked: !chat.liked }; | ||
} else { | ||
return chat; | ||
} | ||
}); | ||
}); | ||
}; |
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 notes on this! Shows me that you have a great understanding of lifting up state!
|
||
|
||
const App =() => { | ||
const [chat, setChat] = useState(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.
Maybe we could change this from chat
to chats
since it is multiple chat messages instead of one?
<ChatLog | ||
entries={chat} | ||
onLiked={handleLikeButtom} | ||
localSender={localSender} | ||
likes={totalLikes} />} |
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.
Does totalLikes
actually need to be passed? We are only using it in line 44
.
const ChatLog = (props) => { | ||
const chatentrycomponents = props.entries.map((entries, index) => { | ||
return ( | ||
<ul 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.
Since we have the id
s of entries we could actually use that. We want to avoid using the indexes.
|
||
|
||
const ChatLog = (props) => { | ||
const chatentrycomponents = props.entries.map((entries, 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.
We could maybe name this entry
instead of entries
since it only a single entry
we will be looking at each iteration.
entries: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
id: PropTypes.number.isRequired, | ||
sender: PropTypes.string.isRequired, | ||
body: PropTypes.string, | ||
timeStamp: PropTypes.string.isRequired, | ||
liked: PropTypes.bool.isRequired, | ||
}) | ||
).isRequired, | ||
|
||
onLiked: PropTypes.func.isRequired, | ||
localSender: PropTypes.string.isRequired, |
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.
⭐️
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 see that the tests are saying that some of the props are undefined but that isn't the case. I'm not entirely sure why.
const likeButtonCliked = () => { | ||
props.onLiked(props.id); | ||
}; |
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.
The button function could also be implemented like so:
<button className='like' onClick={() => likeButtonClicked(id)}>{liked ? '❤️': '🤍'}</button>
const heartColor = props.liked ? '❤️' : '🤍'; | ||
const messageClass = props.sender === props.localSender ? 'local' : '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.
Ternaries, love to see them!
No description provided.