Chatlog - Jennifer N. #127
Conversation
There was a problem hiding this comment.
The tests are all passing and your code looks good. There were some warnings in the tests due to the test data. In larger applications, we'd want to make sure that the tests reflect the anticipated usage, or that our components can otherwise handle missing properties that aren't required. Looking into test warnings can also help us catch missing React best practices, like including key attributes on any component in a list. Nice job keeping your state to the minimum so that we maintain our single source of truth.
| @@ -0,0 +1,23 @@ | |||
| # See https://help.github.com/articles/ignoring-files/ for more about ignoring files. | |||
There was a problem hiding this comment.
It looks like you may have run yarn create react-app chatlog after cloning the project, which wasn't necessary. The project was already setup, with the source files under the src directory. To avoid confusion, I would delete this whole chatlog folder.
| import "@testing-library/jest-dom/extend-expect"; | ||
| import ChatEntry from "./ChatEntry"; | ||
| import { render, screen, fireEvent, waitFor } from "@testing-library/react"; | ||
| import React from 'react'; |
There was a problem hiding this comment.
Looks like prettier reformatted this file when you saved it at some point.
| import "@testing-library/jest-dom/extend-expect"; | ||
| import ChatLog from "./ChatLog"; | ||
| import { render, screen } from "@testing-library/react"; | ||
| import React from 'react'; |
| @@ -1,22 +1,35 @@ | |||
| import React from 'react'; | |||
| import './ChatEntry.css'; | |||
| import PropTypes from 'prop-types'; | |||
| import TimeStamp from './TimeStamp.js' | |||
|
|
|||
| const ChatEntry = (props) => { | |||
| return ( | |||
| <div className="chat-entry local"> | |||
There was a problem hiding this comment.
The optional alignment feature would require us to be able to pick whether to use the local or remote class here. If the App made note of the sender of the first message, and passed that down as a prop, such as localSender, then we could use that value and the value of the current message's sender to determine whether this was a local or remote message.
| <button className="like">🤍</button> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time"> | ||
| <TimeStamp time={props.timeStamp}/> |
There was a problem hiding this comment.
Nice use of the supplied TimeStamp component to handle displaying the time in human-readable style.
|
|
||
| const App = () => { | ||
| const [chat, setChat] = useState(chatMessages); |
There was a problem hiding this comment.
👍 We need a piece of state to track changes to our messages, initialized from the json file we loaded.
| const [chat, setChat] = useState(chatMessages); | ||
|
|
||
| const updateChat = (id) => { | ||
| setChat(chat.map(entry => { |
There was a problem hiding this comment.
Nice use of the callback style of setter, since our next state for the chat data depends on the previous state of the chat data. And nice use of map to make the copy of our array so that React will see that the state has changed.
| const updateChat = (id) => { | ||
| setChat(chat.map(entry => { | ||
| if (entry.id === id) { | ||
| return {...entry, liked: !entry.liked}; |
There was a problem hiding this comment.
👍 Nice way to copy our modified message while also toggling the like, without modifying the previous state. We should always make a copy of any data being modified when working with React state.
| }; | ||
|
|
||
| const getTotalLiked = () => { | ||
| return chat.reduce((total, entry) => { |
There was a problem hiding this comment.
👍 Nice use of reduce to tally up the liked message count from state we were already tracking, without the need for an additional piece of state.
|
|
||
| const getTotalLiked = () => { | ||
| return chat.reduce((total, entry) => { | ||
| return total + entry.liked; |
There was a problem hiding this comment.
It's a little surprising to add a boolean directly to a number. It works, since JS coerces the boolean into an int (true is 1 and false is 0), but it might be nice to use a ternary to explicitly show the conversion.
No description provided.