Skip to content

C22 Phoenix - Liubov D. #22

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

C22 Phoenix - Liubov D. #22

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b2205c4
Implemented wave-01
LiubovDav Dec 15, 2024
c436ffa
Merge pull request #1 from LiubovDav/wave-01
LiubovDav Dec 15, 2024
2542b4b
Implemented wave-02
LiubovDav Dec 15, 2024
867ddf2
Merge pull request #2 from LiubovDav/wave-02
LiubovDav Dec 15, 2024
cb0a381
Added id, key, propTypes
LiubovDav Dec 16, 2024
65cd2f1
Implemented wave-03
LiubovDav Dec 16, 2024
c6b9f45
Merge pull request #3 from LiubovDav/wave-03
LiubovDav Dec 16, 2024
358bbba
Implemented local-remote
LiubovDav Dec 16, 2024
53a5705
Merge pull request #4 from LiubovDav/enhancements
LiubovDav Dec 16, 2024
d9eb229
Changed is required
LiubovDav Dec 16, 2024
19d6bad
Merge pull request #5 from LiubovDav/removed-is-required
LiubovDav Dec 16, 2024
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: 31 additions & 3 deletions src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,42 @@
import './App.css';
import ChatLog from './components/ChatLog';
import chatLogDataFromFile from '../src/data/messages.json';
Copy link

Choose a reason for hiding this comment

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

This file is alreasy in src, so we don't need to move up a folder and come back down again

import chatLogDataFromFile from './data/messages.json';

import { useState } from 'react';

const App = () => {
const [likesCount, setLikesCount] = useState(0);
Copy link

Choose a reason for hiding this comment

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

👀 It's not necessary to introduce additional state to track the total number of likes, and really we would prefer to avoid doing so. The total number of likes can be calculated from the message data, since each message tracks whether it's liked. The total number of likes is an aggregate property of the message data.

It can be tempting to introduce additional state since it can seem inefficient to iterate over all the message data just to calculate that value. But keep in mind that we have to do this anyway just to render the message data into components, so the additional cost is minimal, and avoids potential problems that could lead to states getting out of sync. For a small application like this, the risk is small, but as apps get more complex, we should look for ways to reduce the state used as much as possible.


const [chatLogData, setChatLogData] = useState(chatLogDataFromFile);

const toggleChatEntryLiked = (chatEntryId) => {
Copy link

Choose a reason for hiding this comment

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

👍 Since our state is defined here, we also need to define our mutating function here so that it can "see" the setter function. All we need to receive is the id of the message to toggle, which allows us to locate the message to update, and calculate the next state value. We can pass this all the way down to our ChatEntry which handles the click event, passing us the id of the message that was clicked.

const chatLogDataAfterToggle = chatLogData.map(chatEntry => {
Copy link

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 (chatEntry.id === chatEntryId) {
if (chatEntry.liked) {
setLikesCount(likesCount - 1);
} else {
setLikesCount(likesCount + 1);
}
Comment on lines +14 to +18
Copy link

Choose a reason for hiding this comment

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

If we exclude the additional state to track the total likes, we wouldn't need this logic here. We would need a similar check in whatever logic we use to calculate the total on the next render, but being able to remove the extra state makes moving the logic worth it.

return { ...chatEntry, liked: !chatEntry.liked };
} else {
return chatEntry;
}
});

setChatLogData(chatLogDataAfterToggle);
Copy link

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.

    setChatLogData(chatData => chatLogData.map(chatEntry => {
      if (chatEntry.id === id) {
        return {...chatEntry, liked: !chatEntry.liked};
      } else {
        return chatEntry;
      };
    }));

};

return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>
Chat between Vladimir and Estragon
<br />
<br />
{likesCount} ❤️s
</h1>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={chatLogData} onToggleChatEntryLiked={toggleChatEntryLiked}></ChatLog>
</main>
</div>
);
Expand Down
29 changes: 22 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,35 @@
import './ChatEntry.css';
import TimeStamp from './TimeStamp.jsx';
import PropTypes from 'prop-types';
import { useState } from 'react';

const ChatEntry = (props) => {
const [isLiked, setIsLiked] = useState(props.liked);
Copy link

Choose a reason for hiding this comment

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

👀 We don't need (and shouldn't make) an additional piece of state to copy the liked prop. The liked value will already be managed and passed down by the App. It may look like this would update the local state when this happens, but that's not the case. The value passed to useState is used only on the first render, so to keep this local state in sync with the global state, the ChatEntry has to duplicate the logic that also exists in App. This local state violates the idea of lifting state up.


const likeButtonClicked = () => {
setIsLiked(!props.liked);
Copy link

Choose a reason for hiding this comment

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

👀 This logic is only required because of the local state used for the liked value. This duplicates logic also found in App and requires this presentation component to have extra information about how to manage the data for the chat. We should remove the additional state, and then we wouldn't need this logic.

props.onToggleLiked(props.id);
Copy link

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.

};

const ChatEntry = () => {
return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div className={props.sender == 'Vladimir' ? 'chat-entry local' : 'chat-entry remote'} id={props.id}>
Copy link

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?

<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

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. All we need to do is 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 the TimeStamp component doesn't receive children components (it resembles "void" HTML tags such as input or img rather than tags like p or a that can have children). For such components, prefer to write them using "self-closing" style (note the /> to complete the tag rather than a regular >).

<TimeStamp time={props.timeStamp} />

<button className="like" onClick={likeButtonClicked}>{isLiked ? '❤️' : '🤍'}</button>
Copy link

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 data for this message. Rather than tracking a local piece of state, we can and should use the prop value here directly. This would let us remove the local state.

        <button className="like" onClick={likeButtonClicked}>{props.liked ? '❤️' : '🤍'}</button>

Copy link

Choose a reason for hiding this comment

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

👍 We need a wrapper of some kind rather than calling the received callback through props, since our callback function is expecting a message id as its parameter. If we tried to use it directly as the click event handler, React would end up passing it a clink event, since any function registered as an event handler will always be given the event detail information as its argument.

</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,
onToggleLiked: PropTypes.func
Comment on lines +27 to +32
Copy link

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, as you did.

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.

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;
35 changes: 35 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,35 @@
import ChatEntry from './ChatEntry';
import PropTypes from 'prop-types';

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

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 can make debugging a little more tricky. But any of those approaches will work fine.

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

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={chatEntry.sender}
body={chatEntry.body}
timeStamp={chatEntry.timeStamp}
liked={chatEntry.liked}
onToggleLiked={props.onToggleChatEntryLiked}
></ChatEntry>
);
});

return <>{chatEntryComponents}</>;
};

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
})
),
onToggleChatEntryLiked: PropTypes.func
};

export default ChatLog;