Kemal - Tigers #130
Kemal - Tigers #130
Conversation
|
|
||
|
|
There was a problem hiding this comment.
Minor style: while this is more blank lines than I would normally use, there's nothing wrong with using more. I recommend, though, that you pick a consistent number of lines between elements. For example, there are three lines here between the imports and the code, but in ChatEntry there are four.
But of course, having some extra blank lines is a nanoscopic issue...
|
|
||
| const App = () => { | ||
| const [likesCount, setLikesCount] = useState(0); |
There was a problem hiding this comment.
While this version of state rather cleverly avoids having a state variable of all the chat entries. There is a slight downside, though...
Right now all the entries in messages.json have liked set to false, but if we were to imagine that rather taking this from static data we would get this data from an API, it would be possible for some of the entries to have already been liked and thus have a value of true for liked.
However, with this code, those likes would not appear in this number, and in fact, if you could unlike the message, the number would go negative. (In this case you can't "unlike" that message because the true value just gets ignored as I'll explain in a comment in ChatEntry.)
Of course, there's a simple solution to that, and that's to count the likes before making your call to useState, but at the same time, if we were taking in a list of messages from an API, we probably would have to actually maintain the chat entries using useState.
And in that case, rather than having two state variables, one for the entries and one for the count of likes, it would likely be better to only maintain the one for the entries as the number of likes can be determined from the entries.
Having redundant state like that is a common source for bugs as the state that should stay in sync falls out of sync. Not necessarily enough to be a bug, but often what a reviewer would call a "code smell".
But we're well in hypothetical world at this point, just wanted to point that out. I think for our case your implementation is fine!
|
|
||
| const ChatEntry = (props) => { | ||
| const [likeButton, setLikeButton] = useState('🤍') |
There was a problem hiding this comment.
As mentioned, since the like button setting gets set here to be the empty heart regardless of the actual value in props, if one of the messages had already been liked, then it would still have the empty heart.
But like I said above in App, this doesn't really impact our case, but would have the same issues I outlined if it did.
|
|
||
|
|
There was a problem hiding this comment.
There's no PropTypes set for this component, which is probably fine as it would be redundant with the ones you do set in ChatEntry. At the same time, it's usually a good habit to use tools like PropTypes to check your expectations, because bugs can crop up in surprising places...
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, |
There was a problem hiding this comment.
I would make sure to include updateLikes in these PropTypes just since it is a large part of making this code work, so leaning on PropTypes to catch errors for you is worth the extra typing. (I guess this is what you were planning with the comment on line 37, but I'm still going to point it out.)
I would even go further and mark updateLikes as .isRequired, though this has the downside of making our test fail due to how we (the instructors) wrote the test data. So fair enough if you did not mark it .isRequired.
No description provided.