Skip to content

ChatLog - Luqi Xie C22 Pheonix #29

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

ChatLog - Luqi Xie C22 Pheonix #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e2084e9
complete wave1 and pass test wave1
shiqipaper Dec 16, 2024
7e800f4
complete wave2 to add chatlog component and pass test
shiqipaper Dec 16, 2024
cf4984f
complete wave3 and pass tests
shiqipaper Dec 16, 2024
789eb1a
remove onLike isRequire to fix warning
shiqipaper Dec 16, 2024
389b3e2
Initial commit
shiqipaper Dec 16, 2024
877976e
deploy
shiqipaper Dec 16, 2024
57c8b3f
deployed successfully
shiqipaper 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
40 changes: 40 additions & 0 deletions dist/assets/index-D8dykwLT.js
View file
Open in desktop
Copy link

@anselrognlie anselrognlie Jan 8, 2025

Choose a reason for hiding this comment

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

It should be possible to add dist to the .gitignore so the dist folder doesn't get checked in to your code branch. Build products generally shouldn't get checked into the source repo.

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions dist/assets/index-NRQJTqK-.css
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions dist/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<!doctype html>
<html lang="en">
<head>
<meta charset="UTF-8" />
<link rel="icon" type="image/svg+xml" href="/vite.svg" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<title>React Chat Log</title>
<script type="module" crossorigin src="/react-chatlog/assets/index-D8dykwLT.js"></script>
<link rel="stylesheet" crossorigin href="/react-chatlog/assets/index-NRQJTqK-.css">
</head>
<body>
<div id="root"></div>
</body>
</html>
21,927 changes: 18,124 additions & 3,803 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 7 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,21 @@
"private": true,
"version": "0.0.0",
"type": "module",
"homepage": "https://shiqipaper.github.io/react-chatlog",
Copy link

@anselrognlie anselrognlie Jan 9, 2025

Choose a reason for hiding this comment

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

Since you set the base in vite.config.js it shouldn't be necessary to set homepage here. I'm not sure what effect if any this has for a vite build.

"scripts": {
"dev": "vite",
"build": "vite build",
"lint": "eslint . --report-unused-disable-directives --max-warnings 0",
"preview": "vite preview",
"test": "vitest --run"
"test": "vitest --run",
"deploy": "gh-pages -d dist"
},
"dependencies": {
"luxon": "^2.5.2",
"react": "^18.3.1",
"react-dom": "^18.3.1"
"react-dom": "^18.3.1",
"predeploy": "npm run build",
Copy link

@anselrognlie anselrognlie Jan 8, 2025

Choose a reason for hiding this comment

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

👀 This line "predeploy" is intended to define a script, but is listed in the dependencies section. This prevented npm from being able to install the project. This should be moved up to the "scripts" section.

"react-scripts": "^5.0.1"
},
"devDependencies": {
"@eslint/compat": "^1.2.0",
Expand All @@ -30,6 +34,7 @@
"eslint-plugin-react-hooks": "^5.0.0",
"eslint-plugin-react-refresh": "^0.4.12",
"eslint-plugin-vitest-globals": "^1.5.0",
"gh-pages": "^6.2.0",
"globals": "^15.11.0",
"jest-extended": "^4.0.2",
"jsdom": "^25.0.1",
Expand Down
31 changes: 28 additions & 3 deletions src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,39 @@
import './App.css';
import ChatLog from './components/ChatLog';
import CHATDATA from './data/messages.json';
import { useState } from 'react';


const App = () => {
const [chatData, setChatData] = useState(CHATDATA);

const toggleLike = (id) => {
Copy link

@anselrognlie anselrognlie Jan 10, 2025

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.

setChatData(chatData => chatData.map(chat => {
Copy link

@anselrognlie anselrognlie Jan 10, 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.

Copy link

@anselrognlie anselrognlie Jan 10, 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 (chat.id === id) {
return {...chat, liked: !chat.liked };
Copy link

@anselrognlie anselrognlie Jan 10, 2025

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.

} else {
return chat;
}
}));
};

const calculateTotalLikes = (chatData) => {
return chatData.reduce((total, chat) => {
return total + chat.liked;
Copy link

@anselrognlie anselrognlie Jan 10, 2025

Choose a reason for hiding this comment

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

This works as is, since liked is a boolean value, and JS will coerce true to 1 and false to 0. Still, it can be surprising to see this. Consider using the liked boolean value to determine whether to increment the count

      return chat.liked ? total + 1 : total;

or to explicitly pick the value to add

      return total + (chat.liked ? 1 : 0)

}, 0);
};

const totalLikes = calculateTotalLikes(chatData);
Comment on lines +20 to +26
Copy link

@anselrognlie anselrognlie Jan 10, 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.

I like that you wrapped your reduce call in a well-named helper function. This is one way that we can make using reduce a little more understandable for other programmers reading our code, since the syntax can be a little confusing. Another way to make this self-describing is to give a clear name to the function we pass to reduce.

const sumLikes = (total, chat) => {
  return chat.liked ? total + 1 : total;
};

const totalLikes = chatData.reduce(sumLikes, 0);



return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat between Vladimir and Estragon</h1>
<h2>{totalLikes} ❤️s</h2>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={chatData} onLike={toggleLike}/>
</main>
</div>
);
Expand Down
22 changes: 16 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,30 @@
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';


const ChatEntry = ({ id, sender, body, timeStamp, liked, onLike }) => {
Copy link

@anselrognlie anselrognlie Jan 8, 2025

Choose a reason for hiding this comment

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

👍 I like using destructured props to make it more visibly clear in the function definition itself what props we're expecting to receive. We do need to remember that these are all passed in as a single object (the one we usually call props) 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.

const heartColor = liked ? '❤️' : '🤍';
Copy link

@anselrognlie anselrognlie Jan 10, 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.


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

@anselrognlie anselrognlie Jan 8, 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. 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={timeStamp} />

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

@anselrognlie anselrognlie Jan 10, 2025

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

@anselrognlie anselrognlie Jan 8, 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, 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;
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 = ({ entries, onLike }) => {
const chatComponents = entries.map((entry) => {
Copy link

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

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

@anselrognlie anselrognlie Jan 9, 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}
onLike={onLike}
/>);
});

return (
< section className="chat-log">
Copy link

@anselrognlie anselrognlie Jan 9, 2025

Choose a reason for hiding this comment

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

Nit: leave no space between the < and the tag name.

{chatComponents}
</section>
);
};

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

export default ChatLog;
1 change: 1 addition & 0 deletions vite.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import react from '@vitejs/plugin-react'
// https://vitejs.dev/config/
export default defineConfig({
plugins: [react()],
base: '/react-chatlog/',
test: {
// jest config here
reporters: ['verbose'],
Expand Down