Skip to content

Rachana - Bumblebees #37

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
rachanapatel wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Rachana - Bumblebees #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8778b5a
waves one and two finished
rachanapatel Jun 16, 2025
07b466b
wave three finished
rachanapatel Jun 18, 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
31 changes: 29 additions & 2 deletions src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,41 @@
import './App.css';
import ChatEntry from './components/ChatEntry';
import ChatLog from './components/ChatLog';'./components/ChatLog';
import jsonData from './data/messages.json';
import { useState }from 'react';

const App = () => {
const [messages, setMessages] = useState(jsonData);

const updateMsgLikes = (id) => {
const update = messages.map(message => {
if (message.id === id) {
return { ...message, liked: !message.liked};
}
return message;
});
return setMessages(update)
Comment on lines +11 to +17
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.

In this case, calculating the next version of the message data using the current state variable and passing that updated version to the state setter shouldn't cause any problems, but we still generally prefer using the callback style of setters. Using that approach, we pass a function to the setter whose parameter will receive the latest state value, and which returns the new value to use for the state.

    setMessages(messages => messages.map(message => {
      if (message.id === id) {
        return {...message, liked: !message.liked};
      } else {
        return message;
      };
    }));

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 numHearts = () => {
let count = 0;
for (const msg of messages) {
if (msg.liked === true) {
count++;
}
}
return count;
}
Comment on lines +20 to +28
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.

Also, notice that we could move this helper entirely out of the component if we passed the current messages data as a parameter, rather than using enclosing scope to access it.

  const numHearts = (messages) => {
    return messages.reduce((count, msg) => {
      return msg.liked ? count + 1 : count;
    }, 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.



return (
<div id="App">
<header>
<h1>Application title</h1>
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.

It would be nice to update this to at least a hard-coded representation of the conversation title:

<h1>Chat between Vladimir and Estragon</h1>

<h2>{numHearts()} ❤️s</h2>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={messages} onHeart={updateMsgLikes}></ChatLog>
</main>
</div>
);
Expand Down
26 changes: 19 additions & 7 deletions src/components/ChatEntry.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,32 @@
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';
import { useState } from 'react';

const ChatEntry = (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.

Personally, I like using destructured props to make it more visibly clear in the function definition itself what props we're expecting to receive. Even though we do need to remember that these are all passed in as a single object (your props parameter) and it can cause problems if we forget to include the destructuring syntax (it's easy to forget and list the props as multiple separate params), I really prefer the glanceability.

If you changed your approach to use destructuring here, you'd most likely want to be consistent across all your components.

// const [heart, setHeart] = useState(props.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.

👍 We can figure out which emoji to use for the liked status based on the liked prop without creating any additional state.

let heartShown = props.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. Consider moving even something small like this to a helper function external to the component, so that the behavior of the emoji-picking logic can be tested easily outside of React.


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

Note that for components that don't expect to be given child markup, rather than explicitly writing the closing tag <TimeStamp></TimeStamp>, we prefer to use the empty tag shorthand <TimeStamp />.

{/* <button className="like" onClick={() => setHeart(!heart)}>{heartShown}</button> */}
<button className="like" onClick={() => props.onHeart(props.id)}>{heartShown}</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,
onHeart: 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.

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;
export default ChatEntry;
24 changes: 24 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,24 @@
import './ChatLog.css';
import ChatEntry from './ChatEntry';
import PropTypes from 'prop-types';

const ChatLog = (props) => {
return (
props.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.

While not required in this case (since you are directly returning an array of components, each with a key), consider including a wrapper div around the list of ChatEntry components, as this provides a convenient place to add a className for the list of messages for styling purposes.

<ChatEntry key={entry.id} id={entry.id} sender={entry.sender} body={entry.body} liked={entry.liked} timeStamp= {entry.timeStamp}
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.

We should still aim for lines about 80 columns wide, even when not governed by a recommendation like Python's PEP8. And when our components don't accept nested components (when the tag content body is empty), prefer to omit the closing tag and use the self closing tag syntax (e.g., <MyComponent/> rather than <MyComponent></MyComponent>).

        <ChatEntry
            key={entry.id}
            id={entry.id}
            sender={entry.sender}
            body={entry.body}
            liked={entry.liked}
            timeStamp={entry.timeStamp}
            onHeart={props.onHeart}
        />

onHeart={props.onHeart}></ChatEntry>
));
};

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,
onHeart: PropTypes.func.isRequired,
Comment on lines +14 to +21
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.

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;
4 changes: 3 additions & 1 deletion src/components/TimeStamp.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ const TimeStamp = (props) => {
const absolute = time.toFormat('MMMM Do YYYY, h:mm:ss a');
const relative = time.toRelative();

return <span title={absolute}>{relative}</span>;
return <span title={absolute}>
{relative}
</span>;
};

TimeStamp.propTypes = {
Expand Down