Skip to content

🐲🪰 Mikaela B.#17

Open
mikaebal wants to merge 5 commits into
Ada-C23:mainfrom
mikaebal:main
Open

🐲🪰 Mikaela B.#17
mikaebal wants to merge 5 commits into
Ada-C23:mainfrom
mikaebal:main

Conversation

@mikaebal
Copy link
Copy Markdown

No description provided.

Copy link
Copy Markdown

@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 job on react-chatlog!

Comment thread src/App.jsx
Comment on lines +9 to +11
return chatData.reduce((total, message) => {
return total + (message.liked ? 1 : 0);
}, 0);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice job using reduce! This is how most JS devs would go about calculating a total amount too.

Comment thread src/App.jsx
const totalLikes = calculateTotalLikes(chatData);

const toggleLike = (id) => {
setChatData((chatData) =>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of the callback setter style. In this application, it doesn't really matter whether we use the callback style or the value style, but it's good practice to get in the habit of using the callback style.

Comment thread src/App.jsx

const toggleLike = (id) => {
setChatData((chatData) =>
chatData.map((entry) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice use of map here to both handle making a new list so that React sees the message data has changed, and make new data for the clicked message with its like status toggled.

Comment thread src/App.jsx
setChatData((chatData) =>
chatData.map((entry) => {
if (entry.id === id) {
return { ...entry, liked: !entry.liked };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We showed this approach in class, but technically, we're mixing a few responsibilities here. rather than this function needing to know how to change the liked status itself, we could move this update logic to a helper function. This would better mirror how we eventually update records when there's an API call involved.

In this project, our messages are very simple objects, but if we had more involved operations, it could be worthwhile to create an actual class with methods to work with them, or at least have a set of dedicated helper functions to centralize any such mutation logic.

Comment thread src/App.jsx
Comment on lines +40 to +45
{/* Wave 1: Rendered one ChatEntry
<ChatEntry
sender={chatMessages.sender}
body={chatMessages.body}
timeStamp={chatMessages.timeStamp}
/> */}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This comment might be confusing to other devs because it might make someone wonder if they should uncomment lines 41-44 in order for the ChatEntry component to render.

If you want to keep track of what pieces of data a component should have and how the data should flow between components, creating a diagram or documenting it in the project README could be good places for that and would also leave your code uncluttered with comments.

Suggested change
{/* Wave 1: Rendered one ChatEntry
<ChatEntry
sender={chatMessages.sender}
body={chatMessages.body}
timeStamp={chatMessages.timeStamp}
/> */}

Comment thread src/App.jsx
<div id="App">
<header>
<h1>Application title</h1>
<h1>iYap</h1>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🤗

<button className="like">🤍</button>
<p>{body}</p>
<p className="entry-time">
<TimeStamp time={timeStamp} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice job making use of the provided TimeStamp component.

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