Conversation
|
|
yangashley
left a comment
yangashley
left a comment
There was a problem hiding this comment.
Nice work on react-chatlog! Let me know if you have questions about my comments
| @@ -1,19 +1,43 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
If you look at this "Files changed" section of your pull request it says that 1472 files were committed and pushed to the repo. It looks like you created a virtual environment and then pushed up all the files associated with it. React applications do not need a virtual environment (but Flask projects do). Be mindful of what files you push up to a repository and add to a PR.
It can be helpful to use git add SpecificFileName.js instead of using git add . so that you don't accidentally add and commit files you don't mean to.
In industry, the person reviewing your PR would ask that you resubmit this PR without the venv directory and files so you'd need to figure out how to remove them from your commit history and push the changes again.
| import { useState } from 'react'; | ||
| import './App.css'; | ||
| import chatMessages from './data/messages.json'; | ||
| import ChatLog from './components/ChatLog.js'; |
There was a problem hiding this comment.
This file name is ChatLog.js here, but the name is actually Chatlog.js which prevents the code from compiling. Did you run into any errors when trying to push up the ChatLog.js to your github repo?
| import chatMessages from './data/messages.json'; | ||
| import ChatLog from './components/ChatLog.js'; | ||
|
|
||
| const chatDataList = chatMessages |
There was a problem hiding this comment.
There's no need to use another variable to reference chatMessages. On line 4 when you import the data from './data/messages.json' you reference it with chatMessages so from there on out you can just refer to the data with chatMessages.
| const chatDataList = chatMessages | ||
|
|
||
| const App = () => { | ||
| const [chatData, setChatData] = useState(chatDataList); |
There was a problem hiding this comment.
Instead of setting state with chatDataList, you can set the state with chatMessages directly
|
|
||
| const calcHearts = (chatData) => { | ||
| return chatData.reduce((total, message) => { | ||
| return total + message.liked; |
There was a problem hiding this comment.
Way to use reduce here!
It's cool that you leveraged the knowledge that in JavaScript if you add a boolean to something it gets cast as a number (0 for false, 1 for true). Prefer you use conditional logic to add a number to a number instead, like:
return chat.liked ? total + 1 : total;| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = (props) => { | ||
| const heartColor = props.liked ? '❤️' : '🤍'; |
| <h2 className="entry-name">{props.sender}</h2> | ||
| <section className="entry-bubble"> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time"><TimeStamp time={props.timeStamp}/></p> |
There was a problem hiding this comment.
Nice use of the provided TimeStamp component
2 participants
I was able to get all the test to pass on my end and yarn start. when I look at the file names on git it says Chatlog.js but in VS code it says ChatLog.js. I am not sure why it's not pushing over to github.
