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

react chatlog proj #106

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

react chatlog proj #106

wants to merge 3 commits into from

Conversation

sujinshin8
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.

Hi Sujin, good work on utilizing props and passing them between components. You have a few failed tests which primarily had to do with some of the creative deviations you made for this project. The one important item you're missing is counting the total likes!

Let's meet in a 1:1 to go over this part?

Grade: 🌕

Comment on lines +5 to +7
--Implement a `ChatLog` component and
--update the `App` component to display an entire chat log.
--`ChatLog` should display a sequence of individual `ChatEntry` components.

Choose a reason for hiding this comment

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

Did you mean to make this change? In industry, you'll be submitting PR's with code you'd like your entire team to have. Is this change necessary? If not, we can be warier on what file changes we git add. So instead of using, git add . to add all files we changed locally, we can specify which file we want to add to our commit via git add nameOfFile.js

@@ -5,6 +5,7 @@
In this wave we will update the components to manage a **like** feature.

- Add behavior to heart button in `ChatEntry` so that when it is clicked it toggles from an empty heart (🤍) to a filled heart (❤️) and from a filled heart (❤️) to an empty heart (🤍).

Choose a reason for hiding this comment

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

This change was not necessary either. The previous comment also applies here.

Comment on lines +7 to +19
const [messageData, setMessageData] = useState(chatMessages)

const updateMessageData = (updatedMessage) => {
const messages = messageData.map((message) => {
if (message.id === updatedMessage.id) {
return updatedMessage;
} else {
return message;
}
});

setMessageData(messages)
}

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!

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 +16 to +24
// console.log({
// id,
// sender,
// body,
// timeStamp,
// liked,
// onUpdate
// })
// const firstMessage = props.messages[0]

Choose a reason for hiding this comment

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

In industry, you'll be submitting PR's containing code you'd like your team members to have. In this case, is it necessary for your teammates to have this console.log ? If not, we can remove this line.

<section>{messageComponents}</section>
);
};

Choose a reason for hiding this comment

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

Don't forget to add PropTypes to help validate our props and avoid future errors.

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

Comment on lines +9 to +10


Choose a reason for hiding this comment

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

Remove unneeded whitespace

Comment on lines +35 to +58
// const button = document.getElementById('button');

// if (!props.liked === true){
// button.textContent = '🌻'
// } else {
// button.textContent = '🤍'
// }

// const emojiChange = () => {
// if (!props.liked === true){
// button.textContent = '🌻'
// } else {
// button.textContent = '🤍'
// }
// }

}

// not right
// const button = document.getElementById('button');
// this is not right.....
// const emojiChange = props.liked ? button.textContent = '🌻' : button.textContent = '🤍';
// experimenting
// const emojiChange = props.liked ? 'green' : 'red';

Choose a reason for hiding this comment

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

Let's make sure we remove commented out code that we don't want our teammates to pull.

Comment on lines +11 to +24
const messageComponents = messages.map((message,index) => {
return(
<div key={message.id}>
<ChatEntry
id={message.id}
sender={message.sender}
body={message.body}
timeStamp={message.timeStamp}
liked={message.liked}
onUpdate={onUpdateMessage}
></ChatEntry>
</div>
)
});

Choose a reason for hiding this comment

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

Good work creating multiple sibling components!!

Comment on lines +63 to +67
if (liked === true){
icon = '🌻'
} else {
icon = '🤍'
}

Choose a reason for hiding this comment

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

One of the tests fail because it's expecting a different icon however I don't mind this creative deviance :)

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