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

Lions - Tara Alsaidi #50

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

Conversation

cafedes2moulins
Copy link

No description provided.

…ated updateMessages callback function connected to onClick functionality in ChatEntry, created variable styling for liked functionality and remote vs. local sender
…rack total number of likes in App.js and display this number in the header
Copy link
Contributor

@alope107 alope107 left a comment

Choose a reason for hiding this comment

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

Nice job! This project is green.

Comment on lines +8 to +9
const [messagesList, setMessageList] = useState(chatMessages);
const [likesCounter, setLikesCounter] = useState(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Because we can determine the number of likes from messagesList, likesCounter should not be its own state variable. In general, we want state variables to not depend on one another, otherwise we get multiple sources of truth and increase the risk of bugs.

Comment on lines +24 to +29
if (id === message.id) {
message.liked = !message.liked;
return message;
} else {
return message;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nitpick: the else is not needed here.

import TimeStamp from './TimeStamp';

const ChatEntry = ({ id, sender, body, timeStamp, liked, onLikeClick }) => {
const localOrRemote =
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart!

@@ -1,22 +1,41 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = ({ id, sender, body, timeStamp, liked, onLikeClick }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice destructuring!

Comment on lines +21 to +23
onClick={() => {
onLikeClick(id);
}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice anonymous function!

Comment on lines +31 to +39
messages: PropTypes.arrayOf(
PropTypes.shape({
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.func,
liked: PropTypes.bool.isRequired,
})
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to make the overall array as required as well.

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