Skip to content

BumbleBee-WendyWang#21

Open
wendyww9 wants to merge 14 commits intoAda-C23:mainfrom
wendyww9:main
Open

BumbleBee-WendyWang#21
wendyww9 wants to merge 14 commits intoAda-C23:mainfrom
wendyww9:main

Conversation

@wendyww9
Copy link

No description provided.

Copy link

@kelsey-steven-ada kelsey-steven-ada left a comment

Choose a reason for hiding this comment

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

Really nice work! Let me know if you have any questions on the feedback.

src/App.jsx Outdated

Choose a reason for hiding this comment

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

Great work calculating the like count from the chat data! Since we don't need the contents of the array we create with filter, another option is to use a higher order function like array.reduce to take our list of messages and reduce it down to a single value.

// This could be returned from a helper function
// totalLikes is a variable that accumulates a value as we loop over each entry in chatEntries
const likesCount = chatEntries.reduce((totalLikes, currentMessage) => {
    // If currentMessage.liked is true add 1 to totalLikes, else add 0
    return (totalLikes += currentMessage.liked ? 1 : 0);
}, 0); // The 0 here sets the initial value of totalLikes to 0

Copy link
Author

Choose a reason for hiding this comment

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

Will look into reduce function more.

src/App.jsx Outdated
Comment on lines 6 to 20

Choose a reason for hiding this comment

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

What is the benefit of placing these functions outside of App?

Both functions rely on application specific data and formatting, so I would strongly consider bundling these inside the App function, similar to the chatLiked function.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, will move them inside. Initially, put them outside because we can call them and pass the data to them, so it would be fine if they are outside of App and make App code shorter.

src/App.jsx Outdated
Comment on lines 24 to 34

Choose a reason for hiding this comment

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

Nice work ensuring we're creating a new array of messages to trigger the re-render after updating the liked value!

Comment on lines 26 to 32
return chatData.map(chat => {
if (chat.id === id) {
return {...chat, liked: !chat.liked};
} else {
return chat;
}
});

Choose a reason for hiding this comment

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

This is the same under the hood, but since we have a situation where we return either one thing or the other, we could replace the if/else with a ternary operator:

return chatData.map(chat => {
    return (id === chat.id) ? {...chat, liked: !chat.liked} : chat;
});

Copy link
Author

Choose a reason for hiding this comment

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

Nice! Will try this way.

src/App.jsx Outdated

Choose a reason for hiding this comment

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

Since this is a user facing piece of text, we should prefer to use a semantic element like p to hold the text and use CSS to style it as desired.

Copy link
Author

Choose a reason for hiding this comment

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

Will update it to p element. Thank you.

src/App.jsx Outdated

Choose a reason for hiding this comment

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

These can be a little easier to read with adjusted indentation and using the self-closing tag:

<ChatLog
    entries={chatData}
    onLike ={chatLiked}
    uniqueSenders = {uniqueSenders}
/>

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for pointing it out. Will update.

<p className="entry-time">
<TimeStamp time={props.timeStamp}></TimeStamp>
</p>
<button className="like" onClick={() => props.onLike(props.id)}>{nameColor}</button>

Choose a reason for hiding this comment

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

I like this pattern of sending just the id to props.onLike since it keeps all the state management and message object creation confined to App.

src/App.jsx Outdated

Choose a reason for hiding this comment

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

chatLiked is named in a way that many developers might assume the name represents a boolean variable rather than a function. Including a verb that indicates action is very typical for function names and helps other developers make safer assumptions when reading code. Names like like setChatLiked or updateLikedValue help indicate that the variable is something that takes action over holding a static value.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the feedback. Will look into how to name variables and functions more.

Choose a reason for hiding this comment

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

Nice use of the entry.id for list item key values since our messages have unique ids!

Comment on lines 13 to 23

Choose a reason for hiding this comment

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

There are extra layers to the HTML that aren't necessary. Right now ChatEntry is wrapped in an li at the ChatLog layer and gets wrapped in another div at the ChatEntry layer.

We could remove the li tags in ChatLog and replace the outermost div in ChatEntry with li so that we do not use a div at all, factoring out elements that are not semantic and removing a layer of wrapping!

Copy link
Author

Choose a reason for hiding this comment

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

Got it. Will update this part and work more on refactoring code.

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

Comments