Skip to content

Angelica R - Sea Turtles #89

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
wet-bulb wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Angelica R - Sea Turtles #89

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 23 additions & 4 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,35 @@
import React from 'react';
import React, { useState } from 'react';
import './App.css';
import chatMessages from './data/messages.json';
import ChatLog from './components/ChatLog.js';

const App = () => {
const [messageData, setMessageData] = useState(chatMessages);
const updateMessageData = (updatedMessage) => {
const messages = messageData.map((message) => {
Copy link

@anselrognlie anselrognlie Jun 29, 2022

Choose a reason for hiding this comment

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

Nice job merging the updated record into the new list.

If the logic to flip the liked value were up here, instead of injecting the passed object, we would spread the matching object, and update the liked value in the copy.

if (message.id === updatedMessage.id) {
return updatedMessage;
} else return message;
});
setMessageData(messages);
};
const totalLikes = () => {
Copy link

@anselrognlie anselrognlie Jun 29, 2022

Choose a reason for hiding this comment

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

Nice helper to calculate the number of liked messages.

This would be a great situation to use reduce too!

let likes = 0;
for (const message of messageData) {
if (message.liked) {
likes += 1;
}
}
return likes;
};
return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>ChatLog</h1>
<h2>{totalLikes()} ❤️s</h2>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={messageData} onUpdateMessage={updateMessageData} />
</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('🤍');
});
});
31 changes: 26 additions & 5 deletions src/components/ChatEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,43 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = (props) => {
const buttonEmoji = props.liked ? '❤️' : '🤍';
Copy link

@anselrognlie anselrognlie Jun 29, 2022

Choose a reason for hiding this comment

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

Nice use of ternary for calculating which heart to show based on whether the entry is liked.

const changeLiked = () => {
const updatedMessage = {
id: props.id,
body: props.body,
sender: props.sender,
timeStamp: props.timeStamp,
liked: !props.liked,
};
props.onUpdate(updatedMessage);
Copy link

@anselrognlie anselrognlie Jun 29, 2022

Choose a reason for hiding this comment

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

I tend to prefer the approach of just sending the id to the update method. If we have complex objects, we might not always pass down everything needed to make a new instance down to the control that's displaying it. I also like to think of the presentational component as just know that some part of it was interacted with, and it can report that occurred, but it doesn't necessarily need to know what to do.

Knowing that the liked value needs to be toggled is part of the business logic of the application, and as we have larger applications, we'd like to be able to refactor that sort of code (business logic) out of the react components as much as possible so that they can be tested more easily, and possibly reused across different kinds of projects.

};
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} />
Copy link

@anselrognlie anselrognlie Jun 28, 2022

Choose a reason for hiding this comment

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

👍 Good use of the supplied TimeStamp component.

</p>
<button className="like" onClick={changeLiked}>
{buttonEmoji}
</button>
</section>
</div>
);
};

ChatEntry.propTypes = {
//Fill with correct proptypes
id: PropTypes.number.isRequired,
body: PropTypes.string.isRequired,
sender: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired,
timeStamp: PropTypes.string.isRequired,
onUpdate: PropTypes.func.isRequired,
Comment on lines +35 to +40
Copy link

@anselrognlie anselrognlie Jun 28, 2022

Choose a reason for hiding this comment

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

The ChatEntry tests only know about sender, body, and timeStamp. Marking any other properties as required causes warnings in the ChatEntry tests, as well as the ChatLog tests, since the LOG list in that test file also only includes sender, body, and timeStamp in the data. We could either relax the properties from being required (we should also make sure our components don't crash if non-required data is missing) or update the tests to pass all the data we need.

};

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();
});
});
24 changes: 24 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,24 @@
import React from 'react';
import './ChatLog.css';
import ChatEntry from './ChatEntry';

const ChatLog = (props) => {
const messages = props.entries;
const getChatLogJSX = (messages) => {
return messages.map((message) => {
return (
<ChatEntry
id={message.id}
Copy link

@anselrognlie anselrognlie Jun 29, 2022

Choose a reason for hiding this comment

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

We should add a key to any components that we render into a list. Here, the ideal key is the message id (key={message.id}), though the tests sadly don't pass a message id in with its sample data. 😭

For the tests to be happy, we might optimistically assume that the timestamps are unique (at least they're included in the test data!) and use those as the key. Or we could use the 2 parameter version of map, in which the index gets passed in as the second argument

messages.map((message, index) => { ...

And use the index as the key. In general, this is the least preferable method and should be avoided if the data in the list can change (best option is a primary key from a database record.)

sender={message.sender}
body={message.body}
timeStamp={message.timeStamp}
liked={message.liked}
onUpdate={props.onUpdateMessage}
/>
);
});
};
return <div className="chat-log">{getChatLogJSX(messages)}</div>;
};

Copy link

@anselrognlie anselrognlie Jun 29, 2022

Choose a reason for hiding this comment

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

Remember to add propTypes for the ChatLog component.

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