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

Jamie Horvath C17 Sharks digital #109

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

Conversation

glassofcat
Copy link

No description provided.

Copy link

@audreyandoy audreyandoy left a comment

Choose a reason for hiding this comment

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

Great work Jamie! Your code passes all the tests, requirements, and was really easy to read! I have included some feedback in your PR. Let me know if you have any questions.

Grade: 🟢

Comment on lines +10 to +20
const toggleHeart = (id) => {
// count = 0;
console.log({ id });
const newHeartCount = heart.map((message) => {
if (message.id === id) {
return { ...message, liked: !message.liked };
}
return message;
});
setHeart(newHeartCount);
};

Choose a reason for hiding this comment

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

Nice work managing the data and returning a brand new object using the spread operator!

Here's a refresher on updating state in React:

React components re-render whenever there is a change/update to state data. In this case, our data is an object and we are modifying values nested within that object.

Under the hood, React uses Object.is() to compare the previous state object with the one that's been provided viasetMessages(updatedMessages). Object.is() checks if the object has changed and more specifically if the object passed has a different reference in memory. In Javascript, changing the properties and/or values in an object does NOT change the object reference, which is why we must create a new version of our state object (contains a copy of all the properties that weren't updated along with the properties/values that were).

Here is an article with more info: https://www.valentinog.com/blog/react-object-is/

Comment on lines 6 to 26
const ChatEntry = (props) => {
const heartType = props.liked ? '❤️' : '🤍';
const handleToggleHeart = () => {
props.toggleHeartCallback(props.id);
};

return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<h2 className="entry-name">{props.sender}</h2>
<section className="entry-bubble">
<p>Replace with body of ChatEntry</p>
<p className="entry-time">Replace with TimeStamp component</p>
<button className="like">🤍</button>
<p>{props.body}</p>
<p className="entry-time">
<TimeStamp time={props.timeStamp}></TimeStamp>
</p>
<button className="like" onClick={handleToggleHeart}>
{heartType}
</button>
</section>
</div>
);
};

Choose a reason for hiding this comment

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

Nice work handling the likes feature. I also like the way you wrote this component, in the sense of, all logic pertaining to data or invoking other functions remain in the 'brains' of the component while the 'beauty' section in the return statement is only in charge of rendering data.

Comment on lines 28 to 35
ChatEntry.propTypes = {
//Fill with correct proptypes
id: PropTypes.number,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool,
toggleHeartCallback: PropTypes.func.isRequired,
};

Choose a reason for hiding this comment

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

👍

Comment on lines +6 to +21
const ChatLog = (props) => {
const chitChats = props.entries.map((message, index) => {
return (
<ChatEntry
key={index}
id={message.id}
sender={message.sender}
body={message.body}
timeStamp={message.timeStamp}
liked={message.liked}
toggleHeartCallback={props.toggleHeartCallback}
></ChatEntry>
);
});
return <div className="chat-log">{chitChats}</div>;
};

Choose a reason for hiding this comment

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

👍 Nice work including the key for sibling components!

Comment on lines +23 to +34
ChatLog.propTypes = {
entries: PropTypes.arrayOf(
PropTypes.shape({
id: PropTypes.number,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool,
})
).isRequired,
toggleHeartCallback: PropTypes.func,
};

Choose a reason for hiding this comment

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

Nice work setting up the propTypes for validating the array's nested objects.

Comment on lines +1 to +6
const HeartCount = (messages) => {
let count = 0;
messages.forEach((m) => (m.liked ? count++ : m));
return <div>{count} ❤️s</div>;
};
export default HeartCount;

Choose a reason for hiding this comment

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

Nice helper method!!!

{
/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */
<ChatLog entries={heart} toggleHeartCallback={toggleHeart}></ChatLog>

Choose a reason for hiding this comment

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

Because ChatLog doesn't contain any child components, we can change it to a self-closing tag.

          <ChatLog entries={heart} toggleHeartCallback={toggleHeart} />

Choose a reason for hiding this comment

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

Here's more info on when to use self-closing tags: https://stackoverflow.com/questions/48991212/react-component-closing-tag


const toggleHeart = (id) => {
// count = 0;
console.log({ id });

Choose a reason for hiding this comment

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

When you're working with teams you'll want your PR's to only contain code your teammates should 'pull' as part of the codebase. Code that's meant to test features in local environments such as print statements or console logs can all be removed, or kept in a local branch.

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