Skip to content

Chat Log waves#19

Open
NataliaRaz wants to merge 7 commits intoAda-C23:mainfrom
NataliaRaz:main
Open

Chat Log waves#19
NataliaRaz wants to merge 7 commits intoAda-C23:mainfrom
NataliaRaz:main

Conversation

@NataliaRaz
Copy link

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

Comment on lines +8 to +11
// const [entries, setEntries] = useState(chatMessages);
const [entries, setEntries] = useState(
chatMessages.map((msg) => ({ ...msg, liked: msg.liked ?? false }))
);

Choose a reason for hiding this comment

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

We can assume that all msgs in chatMessages will have a property likedso we don't need to iterate each of them. We can directly set the initial stateentriesequal tochatMessages`, like what you have commented out on line 8.

Suggested change
// const [entries, setEntries] = useState(chatMessages);
const [entries, setEntries] = useState(
chatMessages.map((msg) => ({ ...msg, liked: msg.liked ?? false }))
);
const [entries, setEntries] = useState(chatMessages);
);

setEntries(updatedEntries);
};

const totalLikes = entries.filter((entry) => entry.liked).length;

Choose a reason for hiding this comment

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

Great idea to filter down the data to the liked messages and use the list length as the count!

This approach works, but you'll typically see JS devs use reduce to calculate a value like this:

  const calculateTotalLikeCount = (chatData) => {
    return entries.reduce((acc, entry) => {
      return entry.liked ? acc + 1 : acc;
    }, 0);
  };

const toggleLike = (id) => {
const updatedEntries = entries.map((entry) => {
if (entry.id === id) {
return { ...entry, liked: !entry.liked };

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.

};

const totalLikes = entries.filter((entry) => entry.liked).length;
const localSender = "Vladimir";

Choose a reason for hiding this comment

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

This is a constant variable so we should use all capital letters.

Suggested change
const localSender = "Vladimir";
const LOCAL_SENDER = "Vladimir";

<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat Log</h1>

Choose a reason for hiding this comment

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

An element missing from your project is the header "Chat Between Vladimir and Estragon". Here you have "Chat Log".

@NataliaRaz How would you create a header that reflects the screenshot below? It can be done by hardcoding the string in the h1 tag, but I'm wondering how would you programmatically get the names of the participants in the chat?

Please respond to this comment with your answer, thank you!

image

<h2 className="entry-name">{sender}</h2>
<section className="entry-bubble">
<p>{body}</p>
<p className="entry-time"><TimeStamp time={timeStamp} /></p>

Choose a reason for hiding this comment

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

Nice job using 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

Comments