Kunzite - Cindy V.

[civi10]

No description provided.

[anselrognlie]

🎉 Nice job achieving the behavioral objectives of the project. React is challenging and can take several projects to feel comfortable with. I've left comments throughout, so please take a look and let me know in Slack if there's anything you'd like to follow up on.

[anselrognlie]

👀 We don't need (and should avoid introducing) an additional local piece of state to store whether the message this ChatEntry is displaying is liked. One of the values that should get passed in the props is the liked value, and that value (coming all the way from the state stored in App) should get updated when the like button is clicked. We can then calculate which heart emoji to display as follows:

  const heart = liked ? '❤️' : '🤍';  // assumes liked is destructured as other props

We could calculate this local variable here, and use it in the like button below

        <button className="like" onClick={handleLike}>{heart}</button>

Or perform the calculation in the layout (similar to the current approach, but use the props value instead)

        <button className="like" onClick={handleLike}>{liked ? '❤️' : '🤍'}</button>

[anselrognlie]

👀 Our local component here shouldn't need to know how to toggle whether this message is liked. Rather, this component should only pass the id of the message whose like button was clicked to the callback, and that logic should be responsible for determining what the new liked status for the message should be (the liked count could be calculated on the next render). This would help ensure we have a single source of truth (the list of messages), from which we can draw the entire UI.

  const handleLikeToggle = () => {
    // assume the callback function is passed in as onLikeClicked (reporting the click, not a change)
    props.onLikeClicked(id);
  }

  // in App.js
  const handleLikeClicked = (id) => {
    // copy the list of message stored in the App state
    // copy the message indicated by the id
    // flip the liked status for the message
    // update the App state tracking the messages
  };

  // later in App.js
  <ChatLog entries={messages} onLikeClicked={handleLikeClicked} />

[anselrognlie]

👍 Nice use of the TimeStamp component.

[anselrognlie]

👀 Prefer to base this off the liked value available as part of the message data which we can pass in as part of the props (currently, this is using a local piece of state).

[anselrognlie]

Because this proptype is marked isRequired, we're getting warnings in the tests, since the test author didn't know what you were going to call this property, and so it's absent from the props passed in during the tests. To avoid the warnings, we can leave off isRequired.

Ideally, we'd also include code to ensure that any "optional" values are actually present before trying to use them. For example, in the like click handler we could do something like

    if (onLikeChange) {
        onLikeChange(id);  // (id is currently not being passed to this component, but should be)
    }

which would only try to call the function if it's not falsy (and undefined is falsy).

[anselrognlie]

👀 We should pass the liked value of the message into the ChatEntry (both in the destructuring and the prop types here). Ideally, this component shouldn't need to know how to track and update the liked status itself. Rather, it should display the status to match whatever liked value it receives through props.

[anselrognlie]

👍 Nice inline map to generate ChatEntry components from the message data.

[anselrognlie]

👀 We should pass the liked status for the current message down into the ChatEntry.

[anselrognlie]

👀 The array of message data contains information about whether each message is liked, but unless we store the messages in a piece of state and update it in response to user clicks, we won't be able to make use of the liked value in displaying our UI. So rather than having liked state tracked down in ChatEntry per message, we would like to bring the responsibility for managing that state up to the App level, and pass down the current message state to the ChatLog here.

To help motivate this, imagine that the message data was stored in a backend database (similar to the tasks in task list, or the cards in inspiration board). When the liked status changes, we really want to reflect that liked status change in the database, not only in the UI. While we don't have a backend here, we can get as close to that as possible by lifting the state management up to a list of messages here in App.

[anselrognlie]

👍 Nice handling of the singular and plural.

[anselrognlie]

👀 If the list of messages were stored in state here, we would be able to calculate the current number of liked messages using that state data, and would be able to eliminate the separate piece of state and logic tracking the count of likes!

[anselrognlie]

Thanks for spending the time to work through how to update the messages to be tracked in state. I've included some additional examples for comparison as to how we could take this a little further.

[anselrognlie]

Note that we can calculate the total number of liked counts from the current state of the list of messages on each render, rather than managing a separate piece of state for it. It may seem more efficient to track a piece of state for the count, rather than needing to iterate over the list of messages to calculate it on every render, but keep in mind that React will need to iterate over the entire messages array to generate our list of ChatEntry components anyway, so this effectively adds only a small additional sum to work we already need to do. Remember that from a complexity standpoint, it makes no difference whether we iterate once and perform two operations in the loop, or iterate twice and perform a single operation (so looping and summing, then later looping and building ChatEntry components have the same cost, and the summing is negligible). It's true that if there were other state changes that could occur and this forced our app to re-render the entire message list each time, we could start to see performance problems, but React provides ways to help alleviate this. So as a starting point, it's generally best to calculate any values that we can from the fewest possible pieces of state.

In order to do so, we could write a helper function to iterate through the message data and count the number of liked messages, or we can leverage built-in reduce method to help as follows.

  // this code can be placed in the main body of the component, anywhere before the return
  // it creates a local variable that holds the calculated count of liked messages
  // adding a bool to a number treats true as 1 and 0 as false
  const totalLikes = messages.reduce((totalLikes, message) => (totalLikes + message.liked), 0);

[anselrognlie]

If we take the approach of calculating the total liked count from the message list directly, then this logic can be simplified to focus only on updating the liked status of the clicked message. The code as written does this explicitly by duplicating the message list, locating the message that was clicked, duplicating it, updating its liked status, replacing the message in the copied list with the updated message, and setting the message.

We can write this more compactly, and more clearly (at least once we're comfortable with the map function) as follows:

  const handleLikeCount = (id) => {
    setMessage(messages => (  // the current state value will be passed in to our function
      // map will make our new outer list (react needs to see a new list to redraw)
      messages.map(message => (
        // we can avoid using indexing since map passes each value one by one
        // whatever we return will end up at the "current" slot in the new array
        if (message.id === id) {
          // this is the message that was clicked, so make a copy with the liked flipped
          return {...message, liked: !message.liked};
        } else {
          // this message didn't change (it wasn't clicked), so we can return it as is
          return message;
        }
      ));
    ));
  };

Note the use of the callback style of setMessages, which is more robust to asynchronous updates since our code is given the most recent value of the piece of state, which may have diverged from the version of the data that was current at the time of the render that was captured in our callback. Since there's no async code in ChatLog, it's not necessary, but it's useful to get in the habit of using it.

This code may look longer, but it's primarily due to the explanatory comments.