Skip to content
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

Olive - React Chatlog - Sea Turtles #95

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

Olive - React Chatlog - Sea Turtles #95

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
15747c6
destructured props
olive-lavine Jun 21, 2022
0d81594
added id key to chat log
olive-lavine Jun 21, 2022
a162733
started wave 3
olive-lavine Jun 21, 2022
6002339
passing wave 3
olive-lavine Jun 21, 2022
5739cf6
changed local v remote display appearance
olive-lavine Jun 21, 2022
168866e
updated like count and get participants to helper functions
olive-lavine Jun 21, 2022
c18b329
final edits
olive-lavine Jun 22, 2022
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: 36 additions & 4 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,48 @@
import React from 'react';
import React, { useState } from 'react';
import './App.css';
import ChatLog from './components/ChatLog';
import chatMessages from './data/messages.json';

const App = () => {
const [chatEntries, setChatEntries] = useState(chatMessages);

const updateChatEntry = (updatedEntry) => {
const entries = chatEntries.map((entry) => {
if (entry.id === updatedEntry.id) {
return updatedEntry;
Copy link

Choose a reason for hiding this comment

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

Nice managing of data & making sure we return a new object when we need to alter a message in our list.

} else {
return entry;
}
});
setChatEntries(entries);
};

const getLikeCount = () => {
let likeCount = 0;
for (const entry of chatEntries) {
if (entry.liked === true) {
likeCount += 1;
}
}
return likeCount;
};
Comment on lines +20 to +28
Copy link

Choose a reason for hiding this comment

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

Great use of the existing data to calculate the total likes! Another option would be to use the array function reduce:

    return messageData.reduce((totalLikes, message) => {
      // If messages.liked is true add 1 to totalLikes, else add 0
      return (totalLikes += message.liked ? 1 : 0);
    }, 0); // The 0 here sets the initial value of totalLikes to 0


const getParticipants = () => {
const participants = new Set();
for (const entry of chatEntries) {
participants.add(entry.sender);
}
return [...participants].join(' and ');
Comment on lines +31 to +35
Copy link

Choose a reason for hiding this comment

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

Great use of Set to get the unique senders! If we weren't significantly concerned about space, we could consider mapping chatEntries to an array of only the sender values, then use a Set to remove duplicates:

const allNames = chatEntries.map(message => message.sender);
const participants = [...new Set(allNames)];
return participants.join(' and ');

};

return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat between {getParticipants()}</h1>
<h2>{getLikeCount()} ❤️s</h2>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={chatEntries} onLike={updateChatEntry} />
</main>
</div>
);
Expand Down
60 changes: 30 additions & 30 deletions src/App.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,53 @@
import React from 'react'
import App from './App'
import { render, screen, fireEvent } from '@testing-library/react'
import React from 'react';
import App from './App';
import { render, screen, fireEvent } from '@testing-library/react';

describe('Wave 03: clicking like button and rendering App', () => {
test('that the correct number of likes is printed at the top', () => {
// Arrange
const { container } = render(<App />)
let buttons = container.querySelectorAll('button.like')
const { container } = render(<App />);
let buttons = container.querySelectorAll('button.like');

// Act
fireEvent.click(buttons[0])
fireEvent.click(buttons[1])
fireEvent.click(buttons[10])
fireEvent.click(buttons[0]);
fireEvent.click(buttons[1]);
fireEvent.click(buttons[10]);

// Assert
const countScreen = screen.getByText(/3 ❤️s/)
expect(countScreen).not.toBeNull()
})
const countScreen = screen.getByText(/3 ❤️s/);
expect(countScreen).not.toBeNull();
});

test('clicking button toggles heart and does not affect other buttons', () => {
// Arrange
const { container } = render(<App />)
const buttons = container.querySelectorAll('button.like')
const firstButton = buttons[0]
const lastButton = buttons[buttons.length - 1]
const { container } = render(<App />);
const buttons = container.querySelectorAll('button.like');
const firstButton = buttons[0];
const lastButton = buttons[buttons.length - 1];

// Act-Assert

// click the first button
fireEvent.click(firstButton)
expect(firstButton.innerHTML).toEqual('❤️')
fireEvent.click(firstButton);
expect(firstButton.innerHTML).toEqual('❤️');

// check that all other buttons haven't changed
for (let i = 1; i < buttons.length; i++) {
expect(buttons[i].innerHTML).toEqual('🤍')
expect(buttons[i].innerHTML).toEqual('🤍');
}

// click the first button a few more times
fireEvent.click(firstButton)
expect(firstButton.innerHTML).toEqual('🤍')
fireEvent.click(firstButton)
expect(firstButton.innerHTML).toEqual('❤️')
fireEvent.click(firstButton)
expect(firstButton.innerHTML).toEqual('🤍')
fireEvent.click(firstButton);
expect(firstButton.innerHTML).toEqual('🤍');
fireEvent.click(firstButton);
expect(firstButton.innerHTML).toEqual('❤️');
fireEvent.click(firstButton);
expect(firstButton.innerHTML).toEqual('🤍');

// click the last button a couple times
fireEvent.click(lastButton)
expect(lastButton.innerHTML).toEqual('❤️')
fireEvent.click(lastButton)
expect(lastButton.innerHTML).toEqual('🤍')
})
})
fireEvent.click(lastButton);
expect(lastButton.innerHTML).toEqual('❤️');
fireEvent.click(lastButton);
expect(lastButton.innerHTML).toEqual('🤍');
});
});
4 changes: 3 additions & 1 deletion src/components/ChatEntry.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,6 @@ button {

.chat-entry.remote .entry-bubble:hover::before {
background-color: #a9f6f6;
}
}


36 changes: 30 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,46 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = (props) => {
const onLikeButtonClick = () => {
const updatedChatEntry = {
id: props.id,
sender: props.sender,
body: props.body,
timeStamp: props.timeStamp,
liked: !props.liked,
};
props.onLike(updatedChatEntry);
Comment on lines +7 to +15
Copy link

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.onLike 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 /liked/<id> meant to update the 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 ? '❤️' : '🤍';
const displaySide =
props.sender === 'Vladimir' ? 'chat-entry local' : 'chat-entry remote';
Comment on lines +19 to +20
Copy link

Choose a reason for hiding this comment

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

Nice ternary operators & splitting up lines!


return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div className={displaySide}>
Copy link

Choose a reason for hiding this comment

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

Another option would 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 (so we don't repeat chat-entry anywhere):

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

<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} />
</p>
<button className="like" onClick={onLikeButtonClick}>
{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,
Comment on lines +39 to +43
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 PropTypes and isRequired! Especially when working with other folks, I would recommend keeping all the props we can expect to give to a component listed & uncommented, and only use isRequired on the props that MUST be passed for the component to render (or our tests to pass 😆).

};

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

const ChatLog = (props) => {
const chatComponents = props.entries.map((entry) => {
return (
<li>
key={entry.id}
<ChatEntry
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
onLike={props.onLike}
/>
</li>
);
});

return (
<section>
<ul>{chatComponents}</ul>;
</section>
);
};

ChatLog.propTypes = {
entries: PropTypes.arrayOf(PropTypes.object).isRequired,
Copy link

Choose a reason for hiding this comment

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

We can be even more specific with our PropTypes for validation. If we know that we need an array, and that array's objects need to hold certain data, we can check the objects as well! Inside arrayOf we can pass PropTypes.shape() with information about the object's contents. A modified example from stack overflow is below:

list: PropTypes.arrayOf(
    PropTypes.shape({
        id: PropTypes.number.isRequired,
        customTitle: PropTypes.string.isRequired,
        btnStyle:PropTypes.object,
    })
).isRequired,

Source: https://stackoverflow.com/questions/59038307/reactjs-proptypes-validation-for-array-of-objects

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