Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Leidy-Martinez
Copy link

No description provided.

Copy link

@mikellewade mikellewade left a 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!

Comment on lines +7 to +11
const calculateTotalLikeCount = (chat) => {
return chat.reduce((total, chat) => {
return total + (chat.liked ? 1 : 0);
}, 0);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent execution!

Comment on lines +13 to +21
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
}
}

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!

Comment on lines +27 to +37
const handleLikeButtom = (id) => {
setChat(chat => {
return chat.map(chat => {
if (chat.id === id) {
return { ...chat, liked: !chat.liked };
} else {
return chat;
}
});
});
};

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);

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?

Comment on lines +49 to +53
<ChatLog
entries={chat}
onLiked={handleLikeButtom}
localSender={localSender}
likes={totalLikes} />}

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}>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have the ids of entries we could actually use that. We want to avoid using the indexes.



const ChatLog = (props) => {
const chatentrycomponents = props.entries.map((entries, index) => {

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.

Comment on lines +32 to +43
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,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⭐️

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.

Comment on lines +7 to +9
const likeButtonCliked = () => {
props.onLiked(props.id);
};

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>

Comment on lines +11 to +12
const heartColor = props.liked ? '❤️' : '🤍';
const messageClass = props.sender === props.localSender ? 'local' : 'remote';

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants