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

Kassidy Buslach Sharks #98

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

Kassidy Buslach Sharks #98

wants to merge 7 commits into from

Conversation

buslakj
Copy link

@buslakj buslakj commented Jun 22, 2022

No description provided.

Copy link

@yangashley yangashley 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 practicing React! Let me know if you have any questions about the comments.

🟢 for react-chatlog


const App = () => {
console.log(chatMessages);

Choose a reason for hiding this comment

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

Remove debugging print statements

Comment on lines +20 to +23
const likedTrue = chatsMessages.filter((chat) => {
return chat.liked === true;
});
return likedTrue.length;

Choose a reason for hiding this comment

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

I like this logic in a method so that you can invoke the method when line 30 renders

Comment on lines +39 to +44
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
id: PropTypes.number.isRequired,
liked: PropTypes.bool.isRequired,
onChangeLike: PropTypes.func.isRequired,

Choose a reason for hiding this comment

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

👍

};

ChatLog.propTypes = {
chatsMessages: PropTypes.arrayOf(PropTypes.object).isRequired,

Choose a reason for hiding this comment

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

Since we know the shape of the object that is in the array, we should explicitly validate the shape of the object here for an added layer of validation to prevent your protect your program from errors.

Also note that you have .isRequired on arrayOf() but you'll also. need to have .isRequired on the elements inside arrayOf()

ChatLog.propTypes = {
  chatsMessages: PropTypes.arrayOf(PropTypes.shape({
    id: PropTypes.number.isRequired,
    sender: PropTypes.string.isRequired,
    body: PropTypes.string.isRequired,
    timeStamp: PropTypes.string.isRequired,
    liked: PropTypes.bool
  })).isRequired,
  onChangeLike: PropTypes.func.isRequired
};


return likeState;
};

return (
<div className="chat-entry local">

Choose a reason for hiding this comment

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

Here we have the class chat-entry local hardcoded to this div so when I start up your react app, all the dialogue bubbles are on the left side. We need a way to dynamically change the class between chat-entry local and chat-entry remote.

How would you use a variable to reference the different classes depending on if the ChatEntry belongs to Vladimir or Estragon?

Consider checking to see if the value of sender is 'Vladimir', if it is then the value of the class should be chat-entry local otherwise it should be chat-entry remote. How would you use a ternary to do this?

Comment on lines +13 to +15
return chat;
}
return chat;

Choose a reason for hiding this comment

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

Since you have return chat; inside and outside the if statement, but you need to return chat in both cases, you can delete line 13 and let line 15 do the returning (which will happen regardless of whether line 12 executes or not

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