Skip to content

Bumblebees - Esther K. #28

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.

Sign up for GitHub

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

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

Bumblebees - Esther K. #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6265861
renders one chat entry successfully
essieeekang Jun 15, 2025
2126f69
renders all chat data as entries in a chat log
essieeekang Jun 15, 2025
bef9305
fixes name for data to 'entries' according to tests
essieeekang Jun 15, 2025
bec9d4e
implements liking and unliking messages
essieeekang Jun 16, 2025
12327ad
implements chat entries being on opposite sides depending on sender
essieeekang Jun 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,46 @@
import './App.css';
import ChatLog from './components/ChatLog';
import DATA from './data/messages.json';
import { useState } from 'react';

const calculateLikedCount = (entries) => {
let likedCount = 0;
for (const entry of entries) {
if (entry.liked) {
likedCount++;
}
}
return likedCount;
};
Comment on lines +6 to +14
Copy link

@anselrognlie anselrognlie Jul 7, 2025

Choose a reason for hiding this comment

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

Nice job determining the total likes based on the like data of each message. We don't need an additional piece of state to track this, since it can be derived from the existing state we are tracking.

Explicitly totalling the count is perfectly fine, but many JS programmers would use reduce to achieve this.

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

The first few times we work with reduce, it can be challenging to understand, but it's a tool that gets used commonly enough that it's worth practicing.


const App = () => {
const [entries, setEntries] = useState(DATA);
const likedCount = calculateLikedCount(entries);

const likeEntry = (id) => {
setEntries((entries) => {
return entries.map((entry) => {
Copy link

@anselrognlie anselrognlie Jul 7, 2025

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.

if (entry.id === id) {
return {...entry, liked: !entry.liked};
} else {
return entry;
}
});
});
Comment on lines +21 to +29
Copy link

@anselrognlie anselrognlie Jul 7, 2025

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.

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.

};

return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat Between Vladimir and Estragon</h1>
<section>
<h2 className="widget" id="heartWidget">{likedCount} ❤️s</h2>
</section>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={entries} onLikeEntry={likeEntry}/>
</main>
</div>
);
Expand Down
23 changes: 17 additions & 6 deletions src/components/ChatEntry.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,31 @@
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = ({id, sender, body, timeStamp, liked, onLikeEntry}) => {
const emoji = liked ? '❤️' : '🤍';
Copy link

@anselrognlie anselrognlie Jul 7, 2025

Choose a reason for hiding this comment

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

👍 Nice use of a local variable to store the emoji picked using a ternary.

const entrySender = sender === 'Vladimir' ? 'local' : 'remote';
Copy link

@anselrognlie anselrognlie Jul 7, 2025

Choose a reason for hiding this comment

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

Nice logic to decide whether to treat a message as local or remote. How could we generalize this so that it didn't need to look explicitly for Vladimir? This project only passes in a single conversation, but ideally, our components should work in other situations.

In the general case, does the ChatEntry itself have enough information as it is to "know" which messages are local and which are remote?


const ChatEntry = () => {
return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div className={`chat-entry ${entrySender}`}>
<h2 className="entry-name">{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>{body}</p>
<p className="entry-time"><TimeStamp time={timeStamp}/></p>
Copy link

@anselrognlie anselrognlie Jun 25, 2025

Choose a reason for hiding this comment

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

Nice use of the supplied TimeStamp. We pass in the timeStamp string from the message data and it takes care of the rest. All we had to do was confirm the name and type of the prop it was expecting (which we could do through its PropTypes) and we're all set!

<button className="like" onClick={() => onLikeEntry(id)}>{emoji}</button>
Copy link

@anselrognlie anselrognlie Jul 7, 2025

Choose a reason for hiding this comment

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

👍 Passing the id of this message lets the logic defined up in the App find the message to update in its data.

</section>
</div>
);
};

ChatEntry.propTypes = {
// Fill with correct proptypes
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired,
onLikeEntry: PropTypes.func.isRequired,
Copy link

@anselrognlie anselrognlie Jun 24, 2025

Choose a reason for hiding this comment

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

The id, sender, body, timeStamp, and liked props are always passed (they're defined explicitly in the data and also provided in the test) so we can (and should) mark them isRequired.

The remaining props were up to you, and the tests don't know about them. As a result, using isRequired causes a warning when running any tests that only pass the known props. If you didn't see those warnings when running the tests, be sure to also try running the terminal npm test since the warnings are more visible.

To properly mark any other props isRequired, we would also need to update the tests to include at least dummy values (such as an empty callback () => {} for the like handler) to make the proptype checking happy.

Alternatively, for any props that we leave not required, we should also have logic in our component to not try to use the value if it's undefined.

};

export default ChatEntry;
37 changes: 37 additions & 0 deletions src/components/ChatLog.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import './ChatLog.css';
import PropTypes from 'prop-types';
import ChatEntry from './ChatEntry';

const ChatLog = (props) => {
Copy link

@anselrognlie anselrognlie Jul 7, 2025

Choose a reason for hiding this comment

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

ChatLog uses a single props param, but ChatEntry uses a destructured object. Personally, I prefer the destructured style, since it makes the expected component attributes a bit more clear. And it's fine to use a mixture of styles in this project, but in general, try to pick one style or the other.

const chatEntryComponents = props.entries.map((entry) => {
Copy link

@anselrognlie anselrognlie Jun 25, 2025

Choose a reason for hiding this comment

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

Nice use of map to convert from the message data into ChatEntry components. We can perform this mapping storing the result into a variable we use in the JSX result as you did here (components are functions, so we can run JS code as usual before we reach the return, and even sometimes have multiple return statements with different JSX), we could make a helper function that we call as part of the return, or this expression itself could be part of the return JSX, which I often like since it helps me see the overall structure of the component, though it can make debugging a little more tricky. But any of those approaches will work fine.

return (
<ChatEntry
key={entry.id}
id={entry.id}
Comment on lines +9 to +10
Copy link

@anselrognlie anselrognlie Jun 25, 2025

Choose a reason for hiding this comment

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

👍 The key attribute is important for React to be able to detect certain kinds of data changes in an efficient manner. We're also using the id for our own id prop, so it might feel redundant to pass both, but one is for our logic and one is for React internals (we can't safely access the key value in any meaningful way).

sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
onLikeEntry={props.onLikeEntry}
/>
);
});
return (
<div className="chat-log">
{chatEntryComponents}
</div>
);
};

ChatLog.propTypes = {
entries: PropTypes.arrayOf(PropTypes.shape({
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired
})).isRequired,
onLikeEntry: PropTypes.func.isRequired,
Copy link

@anselrognlie anselrognlie Jun 25, 2025

Choose a reason for hiding this comment

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

Similar to the props for ChatEntry here, the entries prop is included in the tests, but the like toggle is not, resulting in prop warnings (unless we update the tests to reflect our custom props).

Again, if we were to leave this as not required so as to avoid the test warnings, we'd want to be sure that all the script logic in our component worked properly even in the absence of this value.

};

export default ChatLog;