Snow Leopards - Ja H. #124
Conversation
There was a problem hiding this comment.
The tests are nearly all passing (the supplied TimeStamp component was needed to handle formatting the date time of each message) 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. But for this project, it looked fine. Nice job keeping your state to the minimum so that we maintain our single source of truth.
| @@ -1,4 +1,5 @@ | |||
| .chat-log { | |||
| margin: auto; | |||
| max-width: 50rem; | |||
| /* background-color: [orchid]; */ | |||
There was a problem hiding this comment.
To set the background color, this would be
background-color: orchid;The string color name is bare, without any [] wrapping it.
| @@ -3,20 +3,29 @@ import './ChatEntry.css'; | |||
| import PropTypes from 'prop-types'; | |||
|
|
|||
| const ChatEntry = (props) => { | |||
| const heartType = props.liked ? '❤️' : '🤍'; | |||
There was a problem hiding this comment.
Nice job using the liked value coming from props to update the component display. There's no need to track this with local state, since any time we update the state (using the supplied callback) the component data will update, which we will receive through updated props.
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time">{props.timeStamp}</p> |
There was a problem hiding this comment.
We should passed in the timeStamp prop value to the time attribute of the supplied TimeStamp component in order to let it take care of converting the time data into something more human-readable.
| @@ -3,20 +3,29 @@ import './ChatEntry.css'; | |||
| import PropTypes from 'prop-types'; | |||
|
|
|||
| const ChatEntry = (props) => { | |||
| const heartType = props.liked ? '❤️' : '🤍'; | |||
|
|
|||
| 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">{props.timeStamp}</p> | ||
| <button onClick={() => props.onUpdateChat(props.id)} className="like"> |
There was a problem hiding this comment.
Nice use of your passed-in callback to allow parent-owned logic to run when the like button is clicked.
|
|
||
| const App = () => { | ||
| const [chatData, setChatData] = 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 [chatData, setChatData] = useState(chatMessages); | ||
|
|
||
| const updateChatData = (id) => { | ||
| setChatData((chatData) => |
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.
|
|
||
| const updateChatData = (id) => { | ||
| setChatData((chatData) => | ||
| chatData.map((chat) => { |
There was a problem hiding this comment.
Nice use of map to make the copy of our array so that React will see that the state has changed.
| setChatData((chatData) => | ||
| chatData.map((chat) => { | ||
| if (chat.id === id) { | ||
| return { ...chat, liked: !chat.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 findTotalHearts = (chatData) => { |
There was a problem hiding this comment.
👍 Nice helper function to tally up the liked message count from state we were already tracking, without the need for an additional piece of state. We could use reduce to handle the looping part of this. In JS, reduce often fills in for operations like sum, min, or max.
I did not figure out how to get the time stamp to read "x years ago" but I am still working on it.