Tigers - Mica C #114
Tigers - Mica C #114
Conversation
added TimeStamp component into ChatEntry, passing in props.timeStamp
Tracks current number of likes on page, will decrement when unliked
There was a problem hiding this comment.
Good work!
I left a few comments, particularly around how you chose to arrange your state variables, but I think that nonetheless this code is correctly implemented (given our current case), cleanly written, and very readable and maintainable. And thus, I am marking this Green!
| button.liked { | ||
| } |
There was a problem hiding this comment.
Looks like nothing got added to this rule. It happens, but it's usually worth deleting these before checking in.
| const [likeCount, setLikeCount] = useState(0); | ||
| const updateLikeCount = (increment) => { | ||
| setLikeCount(likeCount + increment); | ||
| }; |
There was a problem hiding this comment.
I was about to comment that there was no need to have a likeCount state variable, but then I realized that this 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 unliked the message, the number would go negative.
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!
There was a problem hiding this comment.
Interested in seeing what happened, I cloned your code and set one the messages to have a liked of true and to my surprise (because I haven't looked at ChatEntry yet, that message was displayed as not liked. Which, again, would be fine without my changes, but everything I said in this comment would apply to the heart widget in ChatEntry too.
|
|
||
| const ChatEntry = (props) => { | ||
| const source = props.sender === 'Vladimir' ? 'local' : 'remote'; |
There was a problem hiding this comment.
Good use of the ternary operator! Also good way to distinguish the remote user from the local user!
Further, I think within the hierarchy this is exactly correct place for this check. It wouldn't make sense higher up.
| const [liked, setLiked] = useState(props.liked); | ||
| const toggleLikeButton = () => { | ||
| const heart = document.getElementById(props.id); | ||
| setLiked(!liked); | ||
| if (liked) { | ||
| heart.textContent = '🤍'; | ||
| props.onUpdate(-1); | ||
| } else { | ||
| heart.textContent = '❤️'; | ||
| props.onUpdate(1); | ||
| } | ||
| setLiked(!liked); | ||
| }; |
There was a problem hiding this comment.
Ah, though looking at this, this is a bit of an awkward fallout from the decision above.
To make sure the heart button updates the setting, you have to use useState here to keep track of the liked status. But I notice that your if statement is "backwards" here, and that's because I figure you realized that setLiked does not update the liked variable immediately. Instead these setter functions that useState returns updates on re-render.
And there's nothing "wrong" per se with the logic being backwards here, but it is something that could be a bit of confusion for sure.
I think the best solution would be that rather than always drawing the empty heart and letting it change when the button is clicked, pick which heart to draw based on props.liked. Then you can simply call setLiked here and it will be updated on re-render to the right heart.
There was a problem hiding this comment.
Also there's an extra call to setLiked, which I presume is a typo, and from testing my clone of your code, you can remove it.
| id: PropTypes.number, | ||
| sender: PropTypes.string, | ||
| body: PropTypes.string, | ||
| timeStamp: PropTypes.string, | ||
| liked: PropTypes.bool, |
There was a problem hiding this comment.
If these were .isRequired in ChatLog, they probably should be here too. Now, that being said I know that causes a few test failures, but that's due to our test data not setting these. Either way, completely understand why you may not have set it here, given that.
No description provided.