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.

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

Heather M C17 Sea Turtles #103

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 31 additions & 5 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,42 @@
import React from 'react';
import React, { useState } from 'react';
import './App.css';
import chatMessages from './data/messages.json';
// import ChatEntry from './components/ChatEntry';

Choose a reason for hiding this comment

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

We want to remove commented code before opening PRs, especially in long term projects and when working with other folks to help avoid confusion around why the code is there.

import ChatLog from './components/ChatLog';
import TotalHearts from './components/TotalHearts';
import chatMessagesJSON from './data/messages.json';

const calcTotalLikes = (chatMessages) => {
return chatMessages.reduce((total, message) => {
return total + message.liked;
}, 0);
Comment on lines +9 to +11

Choose a reason for hiding this comment

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

Nice use of the existing data & reduce to calculate the total likes!

};

const App = () => {
const [chatMessages, setMessages] = useState(chatMessagesJSON);

const onLike = (id) => {
const newMessages = chatMessages.map((message) => {
if (message.id === id) {
return { ...message, liked: !message.liked };

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. Great use of the spread operator!

} else {
return message;
}
});
setMessages(newMessages);
};

const totalLikes = calcTotalLikes(chatMessages);

return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat Log</h1>
<h3>
<TotalHearts totalLikes={totalLikes}></TotalHearts>
</h3>
Comment on lines +34 to +36

Choose a reason for hiding this comment

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

In the MDN docs it's highly suggested to not skip headings:

A common navigation technique for users of screen reading software is jumping from heading to quickly determine the content of the page. Because of this, it is important to not skip one or more heading levels. Doing so may create confusion, as the person navigating this way may be left wondering where the missing heading is.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/Heading_Elements#navigation

Here the suggestion would be to use h2 and style it as needed to match the smaller font size desired.

</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={chatMessages} onLike={onLike}></ChatLog>
</main>
</div>
);
Expand Down
39 changes: 34 additions & 5 deletions src/components/ChatEntry.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,51 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
// import chatMessages from '../data/messages.json';
// import { DateTime } from 'luxon';
import TimeStamp from './TimeStamp';

// const messageIndex = 0;

// const timeSince = (messageIndex) => {
// // return message time or how long ago message was sent if more than 1 year ago?
// const timeNow = DateTime.now();
// const messageDate = new Date(chatMessages[messageIndex].timeStamp);
// // 31557600000 ms / year, returns the number of years since message was sent, rounded down.
// const resultTime = Math.floor((timeNow - messageDate) / 31557600000);
// console.log(resultTime);
// return resultTime;
// };

const ChatEntry = (props) => {
const handleLike = () => {
props.onLike(props.id);
Copy link

@kelsey-steven-ada kelsey-steven-ada Jun 30, 2022

Choose a reason for hiding this comment

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

I like this pattern of sending just the id to props. onLike since it keeps all the state management and message object creation confined to App.

};

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">{timeSince(messageIndex)} years ago</p> */}
<p className="entry-time">
<TimeStamp time={props.timeStamp} />
</p>
<button className="like" onClick={handleLike}>
{props.liked ? '❤️' : `🤍`}
</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,
onLike: PropTypes.func,
Comment on lines +43 to +48

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!

};

export default ChatEntry;
32 changes: 32 additions & 0 deletions src/components/ChatLog.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import React from 'react';
import PropTypes from 'prop-types';
import ChatEntry from './ChatEntry';

// const messageIndex = 0;

const ChatLog = (props, onLikeCallback) => {
const chatComponents = props.entries.map((chat, index) => {
return (
<ChatEntry
key={index}

Choose a reason for hiding this comment

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

If our messages have unique ids, another option would be to use the message id as the key rather than an external index.

id={chat.id}
sender={chat.sender}
body={chat.body}
timeStamp={chat.timeStamp}
liked={chat.liked}
onLike={props.onLike}
/>
);
});
return (
<div className="chat-entry local">
<div>{chatComponents}</div>
</div>
);
};

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

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

};

Choose a reason for hiding this comment

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

I recommend listing all the props we can expect to give to a particular component, including functions like onLike.


export default ChatLog;
22 changes: 22 additions & 0 deletions src/components/TotalHearts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
import React from 'react';
import PropTypes from 'prop-types';

const TotalHearts = (props) => {

Choose a reason for hiding this comment

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

I like the idea of having this as it's own component! If we wanted to add features (like maybe showing the like count for each user on hover), it's easy to extend without cluttering our App file.

const hearts = (numberOfLikes) => {
// let heartString = '';
// for (let i = 0; i < numberOfLikes; i++) {
// heartString = `${heartString}❤️`;
// }
// console.log(numberOfLikes, heartString);
// return `${numberOfLikes} ❤️ s ${heartString}`;
return `${numberOfLikes} ❤️s`;
};

return <div className="hearts-count local">{hearts(props.totalLikes)}</div>;

Choose a reason for hiding this comment

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

We need to create some sort of wrapper for our JSX content, so we could use a div here, but since div elements don't hold semantic meaning, we want to use them as sparingly as possible. We could refactor our HTML between App and here to remove this div. One option would be to replace this div with an h3, then remove the h3 that surrounds the TotalHearts component in App. The text remains rendered inside an h3 and gets the CSS rules assigned to it, and we've removed a layer of element nesting that isn't required!

};

TotalHearts.propTypes = {
totalLikes: PropTypes.number.isRequired,
};

export default TotalHearts;