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

Zoisite - Whitney Shake #109

Open
wants to merge 6 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
34 changes: 31 additions & 3 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,44 @@
import React from 'react';
import './App.css';
import chatMessages from './data/messages.json';
import ChatLog from './components/ChatLog';
import { useState } from 'react';

const App = () => {

const [chatData, setChatData] = useState(chatMessages);
const [likeCount, setLikeCount] = useState(0);
Comment on lines +9 to +10

Choose a reason for hiding this comment

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

👍 nice job setting up state for chat messages.

Notice that you create a variable called likeCount to keep track of state that represents likes. You pass likeCount to ChatLog as a prop called likeCount on line 40. However, in ChatLog, the prop likeCount is never accessed. This means you create state, pass it as a prop and then do not use it.

We can remove line 10 because we do not need likes to be tracked as state. We can simply write a method that counts all the hearts from each chat in chatData and returns the count. Then you can use this return value to render the heart count.

Something like:

const getHeartCount = () => {
    return chatData.reduce((total, chat) => {
      return chat.liked ? total + 1 : total;
    }, 0)
};

// then on line 33, you can write this:
<h2 className="widget">{getHeartCount()} ❤️s</h2>



const updateChatData = (updatedChat) => {
const entries = chatData.map((entry) => {
if (entry.id === updatedChat.id) {
if (entry.liked !== updatedChat.liked) {
const likeCountChange = updatedChat.liked ? 1 : -1;
Comment on lines +13 to +17

Choose a reason for hiding this comment

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

Rather than receiving a new entry with it's like status already changed here, rewrite this to accept only an id. The logic about how to copy the message and change the liked status would be better if it was written in App.js, since the state must have all the information necessary to make a new message.

const updateEntryData = id => {
const entries = chatData.map(entry => {
      if (entry.id === id){
        return {...entry, liked: !entry.liked};
      } 
      else {
        return entry;
      }
    });
    setChatData(entries);
}

Also see my above comment about eliminating likeCount as state and creating a separate method for counting likes. When you get rid of likeCount as state, you reduce the complexity of your application.. We've confirmed that likeCount doesn't actually need to be a state since you pass it as a prop but don't use it in the child component. Therefore, you don't need to update the likeCountChange value here. You can just invoke the getHeartCount() method I detailed above

setLikeCount((prevCount) => prevCount + likeCountChange);
}
return updatedChat;
} else {
return entry;
}
});
setChatData(entries);

Choose a reason for hiding this comment

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

👀 Since the next version of the chat data depends on the current version, prefer using the callback style of setChatData:

setChatData(prevEntries => ( 
    // logic to use current chats to make a new copy with the desired liked status flipped (the map logic above) 
    // return the new chats to update the state 
))

};

return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Chat between Vladimir and Estragon</h1>
<section>
<h2 className="widget" id="heartWidget">{likeCount} ❤️s</h2>

Choose a reason for hiding this comment

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

When you write a method that doesn't need likeCount as state, you can just invoke the method getHeartCount() here between the curly braces

</section>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog
entries={chatData}
updateChatData={updateChatData}
likeCount={likeCount}
/>
</main>
</div>
);
Expand Down
1 change: 1 addition & 0 deletions src/App.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ describe('Wave 03: clicking like button and rendering App', () => {
// Arrange
const { container } = render(<App />);
let buttons = container.querySelectorAll('button.like');
console.log(buttons);

Choose a reason for hiding this comment

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

Remember to remove debugging print statements before opening a PR for review. We want to avoid print statements being committed to the code base to keep it readable and concise


// Act
fireEvent.click(buttons[0]);
Expand Down
3 changes: 2 additions & 1 deletion src/components/ChatEntry.css
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
button {
button {
background: none;
color: inherit;
border: none;
Expand All @@ -8,6 +8,7 @@ button {
outline: inherit;
}


.chat-entry {
margin: 1rem;
}
Expand Down
39 changes: 32 additions & 7 deletions src/components/ChatEntry.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,47 @@
import React from 'react';
import './ChatEntry.css';
import TimeStamp from './TimeStamp';
import PropTypes from 'prop-types';

const ChatEntry = (props) => {

const ChatEntry = ({sender, body, timeStamp, liked, id, updateChatData}) => {

const location = sender === 'Vladimir' ? 'local' : 'remote';

Choose a reason for hiding this comment

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

👌

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

const onHeartClick = () => {
const updatedEntry = {
id: id,
sender: sender,
body: body,
timeStamp: timeStamp,
liked: !liked,
};
updateChatData(updatedEntry);
};
Comment on lines +12 to +21

Choose a reason for hiding this comment

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

See my comment in app.js about only sending updateChatData an id as a parameter (instead of a constructed updatedEntry object. That would make onHeartClick() look like:

const onHeartClick = () => {
   updateChatData(id);
};


return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div className={`chat-entry ${location}`}>
<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}/>

Choose a reason for hiding this comment

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

Nice job passing the correct prop to the provided TimeStamp component 👍

</p>
<button className="like" onClick={onHeartClick}>{heartColor}</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,
updateChatData: PropTypes.func,
};

export default ChatEntry;

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

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

return (
<section>
{chatComponents}
</section>
)
Comment on lines +7 to +25

Choose a reason for hiding this comment

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

Instead of creating a variable chatComponents to reference the return value from calling map() on props.entries, it's more common to directly iterate over the chat entries in your component like this without using a variable:

const ChatLog = ({entries, heartClick}) => {
    return (
     <section>
        {props.entries.map((entry) => (
            <ChatEntry 
                key={entry.id} 
                id={entry.id}
                sender={entry.sender}
                body={entry.body}
                timeStamp={entry.timeStamp}
                liked={entry.liked}
                updateChatData={props.updateChatData}
            />
        ))}
    </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,

Choose a reason for hiding this comment

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

Notice that the prop likeCount that was passed to ChatLog on line 40 in App.js isn't part of PropTypes but also isn't referenced in the rest of this component?

})),
updateChatData: PropTypes.func
};

export default ChatLog;