Skip to content

Ocelots - Xuan Hien Pham #12

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

Ocelots - Xuan Hien Pham #12

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7d30378
completed wave 01
HienXuanPham Jan 29, 2023
d7e64b9
completed wave02 and modify the chatleft and right
HienXuanPham Jan 30, 2023
583e37a
added useState in ChatEntry
HienXuanPham Feb 2, 2023
61120fb
completed wave 03
HienXuanPham Feb 2, 2023
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
42 changes: 41 additions & 1 deletion src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,56 @@
import React from 'react';
import { useState } from 'react';
Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 6, 2023

Choose a reason for hiding this comment

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

We could combine the react imports into a single line:

import React, { useState } from 'react';

import './App.css';

import chatMessages from './data/messages.json';
import ChatLog from './components/ChatLog';

const App = () => {
const [chatMessagesData, setChatMessagesData] = useState(chatMessages);

const updateChatMessagesData = (updatedChatMessages) => {
const messages = chatMessagesData.map((chatMessage) => {
if (chatMessage.id === updatedChatMessages.id) {
return updatedChatMessages;
} else {
return chatMessage;
}
});
setChatMessagesData(messages);
};
let count = 0;
Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 7, 2023

Choose a reason for hiding this comment

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

It looks like this variable count is only used inside the function countLike, so we should move the instantiation inside of the function so the variable cannot be affected by other code.

const countLike = () => {
for (let i = 0; i < chatMessagesData.length; ++i) {
if (chatMessagesData[i]['liked'] === true) {
count += 1;
}
}
return count;
};
Comment on lines +22 to +29
Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 7, 2023

Choose a reason for hiding this comment

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

Nice loop to calculate the like count from the message data! Another option is to use a higher order function like array.reduce to take our list of messages and reduce it down to a single value.

// This could be returned from a helper function
// totalLikes is a variable that accumulates a value as we loop over each entry in chatEntries
const likesCount = chatMessagesData.reduce((totalLikes, currentMessage) => {
    // If currentMessage.liked is true add 1 to totalLikes, else add 0
    return (totalLikes += currentMessage.liked ? 1 : 0);
}, 0); // The 0 here sets the initial value of totalLikes to 0

Copy link
Author

@HienXuanPham HienXuanPham Feb 9, 2023

Choose a reason for hiding this comment

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

Thank you! It is an interesting solution. A higher order function is take another function as its parameter. What is it in this case?

Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In this case we have an anonymous function in the inner code that take in the totalLikes and currentMessage:

(totalLikes, currentMessage) => {
    // If currentMessage.liked is true add 1 to totalLikes, else add 0
    return (totalLikes += currentMessage.liked ? 1 : 0);
}


return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat between Vladimir and Estragon</h1>
<section>
<h1 className="widget" id="heartWidget">
{countLike()} ❤️s
</h1>
</section>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
{/* <ChatEntry
sender={chatMessages[0].sender}
body={chatMessages[0].body}
timeStamp={chatMessages[0].timeStamp}
></ChatEntry> */}

<ChatLog
entries={chatMessagesData}
onUpdateChatMessages={updateChatMessagesData}
></ChatLog>
</main>
</div>
);
Expand Down
37 changes: 31 additions & 6 deletions src/components/ChatEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,47 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = (props) => {
const onHeartButtonClick = () => {
const updatedChatMessage = {
id: props.id,
sender: props.sender,
body: props.body,
timeStamp: props.timeStamp,
liked: !props.liked,
};
props.onUpdate(updatedChatMessage);
};
Comment on lines +7 to +16
Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 7, 2023

Choose a reason for hiding this comment

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

I would consider passing the id of the message clicked to props.onUpdate and having the App code handle the new object creation. When ChatEntry creates the new object for the App state, it takes some responsibility for managing those contents. If we want the responsibility of managing the state to live solely with App, we would want it to handle defining the new message object.

This made me think of a related concept in secure design for APIs. Imagine we had an API for creating and updating messages, and it has an endpoint /<msg_id>/like meant to update a true/false liked value. We could have that endpoint accept a body in the request and let the user send an object with data for the message's record (similar to passing a message object from ChatEntry to App), but the user could choose to send any data for those values. If the endpoint only takes in an id and handles updating the liked status for the message itself, there is less opportunity for user error or malicious action.

const heart = !props.liked ? '🤍' : '❤️';
return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div
className={
'chat-entry ' + (props.sender === 'Vladimir' ? 'local' : 'remote')
}
Comment on lines +20 to +22
Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 7, 2023

Choose a reason for hiding this comment

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

Having the decision logic in the JSX may make it harder to read quickly. Another option could be to have an interpolated string here that always holds chat-entry and use a placeholder where we pass only the remote or local class name:

const entryClassName = (props.sender === 'Vladimir') ? 'local' : 'remote';
...
<div className={`chat-entry ${entryClassName}`}>

>
<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>
<button className="like" onClick={onHeartButtonClick}>
{heart}
</button>
</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,
onUpdate: PropTypes.func.isRequired,
};

export default ChatEntry;
16 changes: 8 additions & 8 deletions src/components/ChatEntry.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import React from "react";
import "@testing-library/jest-dom/extend-expect";
import ChatEntry from "./ChatEntry";
import { render, screen, fireEvent, waitFor } from "@testing-library/react";
import React from 'react';
import '@testing-library/jest-dom/extend-expect';
import ChatEntry from './ChatEntry';
import { render, screen, fireEvent, waitFor } from '@testing-library/react';

describe("Wave 01: ChatEntry", () => {
describe('Wave 01: ChatEntry', () => {
beforeEach(() => {
render(
<ChatEntry
Expand All @@ -14,15 +14,15 @@ describe("Wave 01: ChatEntry", () => {
);
});

test("renders without crashing and shows the sender", () => {
test('renders without crashing and shows the sender', () => {
expect(screen.getByText(/Joe Biden/)).toBeInTheDocument();
});

test("that it will display the body", () => {
test('that it will display the body', () => {
expect(screen.getByText(/Get out by 8am/)).toBeInTheDocument();
});

test("that it will display the time", () => {
test('that it will display the time', () => {
expect(screen.getByText(/\d+ years ago/)).toBeInTheDocument();
});
});
35 changes: 35 additions & 0 deletions src/components/ChatLog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import React from 'react';
import ChatEntry from './ChatEntry';
import PropTypes from 'prop-types';

const ChatLog = (props) => {
const chatEntry = props.entries.map((chat) => {
return (
<ChatEntry
key={chat.id}
id={chat.id}
sender={chat.sender}
body={chat.body}
timeStamp={chat.timeStamp}
liked={chat.liked}
onUpdate={props.onUpdateChatMessages}
></ChatEntry>
);
});
return <section className="chat-log">{chatEntry}</section>;
};

ChatLog.propTypes = {
entries: PropTypes.arrayOf(
PropTypes.shape({
Copy link

@kelsey-steven-ada kelsey-steven-ada Feb 7, 2023

Choose a reason for hiding this comment

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

Really nice use of PropTypes.

id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string,
timeStamp: PropTypes.string,
liked: PropTypes.bool.isRequired,
onUpdate: PropTypes.func.isRequired,
}).isRequired
),
};

export default ChatLog;
48 changes: 24 additions & 24 deletions src/components/ChatLog.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,49 +1,49 @@
import React from "react";
import "@testing-library/jest-dom/extend-expect";
import ChatLog from "./ChatLog";
import { render, screen } from "@testing-library/react";
import React from 'react';
import '@testing-library/jest-dom/extend-expect';
import ChatLog from './ChatLog';
import { render, screen } from '@testing-library/react';

const LOG = [
{
sender: "Vladimir",
body: "why are you arguing with me",
timeStamp: "2018-05-29T22:49:06+00:00",
sender: 'Vladimir',
body: 'why are you arguing with me',
timeStamp: '2018-05-29T22:49:06+00:00',
},
{
sender: "Estragon",
body: "Because you are wrong.",
timeStamp: "2018-05-29T22:49:33+00:00",
sender: 'Estragon',
body: 'Because you are wrong.',
timeStamp: '2018-05-29T22:49:33+00:00',
},
{
sender: "Vladimir",
body: "because I am what",
timeStamp: "2018-05-29T22:50:22+00:00",
sender: 'Vladimir',
body: 'because I am what',
timeStamp: '2018-05-29T22:50:22+00:00',
},
{
sender: "Estragon",
body: "A robot.",
timeStamp: "2018-05-29T22:52:21+00:00",
sender: 'Estragon',
body: 'A robot.',
timeStamp: '2018-05-29T22:52:21+00:00',
},
{
sender: "Vladimir",
body: "Notabot",
timeStamp: "2019-07-23T22:52:21+00:00",
sender: 'Vladimir',
body: 'Notabot',
timeStamp: '2019-07-23T22:52:21+00:00',
},
];

describe("Wave 02: ChatLog", () => {
describe('Wave 02: ChatLog', () => {
beforeEach(() => {
render(<ChatLog entries={LOG} />);
});

test("renders without crashing and shows all the names", () => {
test('renders without crashing and shows all the names', () => {
[
{
name: "Vladimir",
name: 'Vladimir',
numChats: 3,
},
{
name: "Estragon",
name: 'Estragon',
numChats: 2,
},
].forEach((person) => {
Expand All @@ -56,7 +56,7 @@ describe("Wave 02: ChatLog", () => {
});
});

test("renders an empty list without crashing", () => {
test('renders an empty list without crashing', () => {
const element = render(<ChatLog entries={[]} />);
expect(element).not.toBeNull();
});
Expand Down