-
Notifications
You must be signed in to change notification settings - Fork 117
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
finished forgot to commit oh no #103
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"python.testing.unittestArgs": [ | ||
"-v", | ||
"-s", | ||
".", | ||
"-p", | ||
"*test.py" | ||
], | ||
"python.testing.pytestEnabled": false, | ||
"python.testing.unittestEnabled": true | ||
} | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,43 @@ | ||
import React from 'react'; | ||
import './App.css'; | ||
import chatMessages from './data/messages.json'; | ||
import ChatLog from './components/ChatLog'; | ||
import { useState } from 'react'; | ||
import Chats from './data/messages.json'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since |
||
|
||
|
||
const App = () => { | ||
const [chatMessages, setChatMessages] = useState(Chats); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 nice job setting up state for chat messages. |
||
|
||
const heartClick = (entryToUpdate) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 method 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 heartClick = id => {
const entries = chatMessageData.map(message => {
if (message.id === id){
return {...message, liked: !message.liked};
}
else {
return message;
}
});
setChatMessages(entries);
} |
||
const entries = chatMessages.map((message) => { | ||
if (message.id === entryToUpdate.id){ | ||
return entryToUpdate; | ||
} | ||
return message; | ||
}); | ||
setChatMessages(entries); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 setChatsLike(chats => (
// 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
)) You can see an example of how we did it in Flasky here |
||
}; | ||
|
||
const countLikes = (chatMessages) => { | ||
let likeCount = 0; | ||
for(const message of chatMessages){ | ||
if (message.liked=== true){ | ||
likeCount++; | ||
} | ||
}; | ||
return likeCount; | ||
}; | ||
Comment on lines
+22
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can use the reduce method here to find the total count of hearts, like: const getHeartCount = () => {
return chatData.reduce((total, chat) => {
return chat.liked ? total + 1 : total;
}, 0)
}; |
||
const heartTotal = countLikes(chatMessages); | ||
return ( | ||
<div id="App"> | ||
<header> | ||
<h1>Application title</h1> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per the wireframes in the project README this should say "Chat between Vladimir and Estragon" |
||
<h2>Total Likes: {heartTotal} ❤️s</h2> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of using the variable <h2>Total Likes: {countLikes()} ❤️s</h2> |
||
</header> | ||
<main> | ||
{/* Wave 01: Render one ChatEntry component | ||
Wave 02: Render ChatLog component */} | ||
<ChatLog entries={chatMessages} heartClick= {heartClick}></ChatLog> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick - there's an extra whitespace that needs to be removed between the equal sign and the left curly brace when you pass in the second prop |
||
</main> | ||
|
||
</div> | ||
); | ||
}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,38 @@ | ||
import React from 'react'; | ||
import './ChatEntry.css'; | ||
import PropTypes from 'prop-types'; | ||
import TimeStamp from './TimeStamp'; | ||
|
||
|
||
const ChatEntry = ({id, sender, body, timeStamp, liked, heartClick}) => { | ||
|
||
const clickLikedHeart = () => { | ||
heartClick( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my comment in app.js about only passing in an id (instead of an entire chat object) into That would make this method look like: const clickLikedHeart = () => {
heartClick(id);
}; |
||
{id:id, | ||
sender:sender, | ||
body:body, | ||
timeStamp:timeStamp, | ||
liked:!liked})}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The last curly brace and semi colon should be on its own line on line 16 so that it's quickly evident to a reader where the function ends. Right now, someone reading your code quickly can't easily see that this function ends on line 15. Like: const clickLikedHeart = () => {
heartClick(
{id:id,
sender:sender,
body:body,
timeStamp:timeStamp,
liked:!liked});
}; |
||
|
||
const ChatEntry = (props) => { | ||
return ( | ||
<div className="chat-entry local"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While it is an optional enhancement to make the local and remote messages display on different sides of the screen, I did want to plant the seed of how you could do that. If you look in ChatEntry.css, there are 2 classes called 'local' and 'remote'. Right now, on line 18 we've hardcoded one of the class names to be 'local'. But what if we checked to see if the
Then you could use senderClass on line 18 to set the class depending on the sender, like:
|
||
<h2 className="entry-name">Replace with name of sender</h2> | ||
<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}></TimeStamp></p> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice job passing the correct prop to the provided TimeStamp component 👍 There's an extra whitespace after the equal sign which doesn't affect the code executing, but we should strive to maintain consistency when writing code to keep it maintainable and readable. |
||
<button className="like" onClick={clickLikedHeart}>{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, | ||
heartClick: PropTypes.func | ||
Comment on lines
+34
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh wait! i remember hearing in the wrap-up session that tests were failing unless you removed .isRequired. Disregard this comment |
||
}; | ||
Comment on lines
29
to
36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 PropTypes look good! |
||
|
||
export default ChatEntry; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
import React from 'react'; | ||
import './ChatLog.css'; | ||
import ChatEntry from './ChatEntry'; | ||
|
||
|
||
|
||
const ChatLog = ({entries, heartClick}) => { | ||
|
||
const getChatLog = () => { | ||
return entries.map((entry) => { | ||
return ( | ||
<ChatEntry | ||
key= {entry.id} | ||
id= {entry.id} | ||
sender= {entry.sender} | ||
body= {entry.body} | ||
timeStamp={entry.timeStamp} | ||
liked={entry.liked} | ||
heartClick= {heartClick} | ||
Comment on lines
+13
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inconsistent whitespaces after = needs to be tidied up before opening a PR |
||
/> | ||
); | ||
}); | ||
}; | ||
return ( | ||
<div> | ||
{getChatLog(entries)} | ||
</div> | ||
Comment on lines
+9
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, instead of writing a method const ChatLog = ({entries, heartClick}) => {
return (
<div>
{entries.map((entry) => (
<ChatEntry
key={entry.id}
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
heartClick={heartClick}
/>
))}
</div>
)
}; |
||
) | ||
}; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This component is missing PropTypes: https://legacy.reactjs.org/docs/typechecking-with-proptypes.html |
||
export default ChatLog; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a blank line was added to the file and then added/committed. we want to avoid bringing in unnecessary changes to a PR. In the future, you can just unstage the change so it doesn't get committed. |
||
[ | ||
{ | ||
"id": 1, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these are your settings for your own vscode, you don't need to add/commit this file