D19 Kunzite - Abby Castillo

[abbymachines]

only finished waves one and two. working on wave 3

[marciaga]

Good job with this project! I like the way you created additional components to encapsulate some discreet functionality, leveraging props to control what they're rendering. Use caution when porting code over though, so as to not break tests (and if you break tests that used to pass, it's advisable to fix them so that they do!). Also, be cautious about mutating objects when working with React and to use state and update functions to let React know when something in the UI should change.

[marciaga]

While it does work to add likeCount to state and call the update function while setting message data, there isn't a need to do so. Because of the way state updates work in React, whenever a state update function is called, the component is re-rendered and therefore every piece of JS in the component is also re-executed. This means that you can compute the like count based on the updated messages either by filtering like

const totalLikes = messages.filter(message => message.liked).length;

or like

const totalLikes = messages.reduce((message, total) => {
  if (message.liked) {
    total += 1;
  }
  return total;
}, 0);

And indeed there's a good reason not to update the like count using state. Because every time an update function is called, the component re-renders, so calling two update functions as you're doing when someone clicks the like button, will cause 2 re-renders. At this small scale, that's not very expensive, but re-rendering a lot of components can start to cause UI lag, which you'll want to avoid.

[marciaga]

In general, when you're updating state that depends directly on the previous state, you should use the argument that React passes to the state update function's callback argument (which is the previous state). For example,

setHeartTotal((previousHeartTotal) => previousHeartTotal + 1);

That way you'll be guaranteed to have the computation based on the actual most recent state.

[marciaga]

Here you should be using state (messageData), rather than the original message JSON. The reason the message "like" toggling appeared to be working is because you were actually mutating the original message objects (see line 20 above for additional comments).

[marciaga]

I like how you decided to create additional components that are controlled by props. It encourages reusability, which is an important thing in React.

[marciaga]

I like this customization, however, it makes some tests fail because the tests are expecting this JSX:

{props.likeTotal} ❤️s

[marciaga]

Again, I like how you created a new component to abstract the like button and control it with props 👍🏽

[marciaga]

This kind of conditional works fine but is a bit repetitive. It would work well to set the heart emoji based on a condition in a ternary and thereby reduce the amount of JSX:

const LikeButton = (props) => (
  <button className="like" onClick={props.updateLike}>
    {props.heartCondition ? '❤️' : '🤍'}
  </button>
);

[marciaga]

Some tests are failing because there is a class that the tests are using in order to be selected and fire click events on (see line 24 in App.test.js). If you add the like to the className prop, the tests will be able to select like buttons correct again. I think possibly you might have forgotten to carry it over when creating this component, because it was in the JSX from the start. It would also allow you to apply the style that you added in line 104 of ChatEntry.css (since it's selecting the like class).

[marciaga]

Typically, we'd wrap a component in additional JSX if there was a structural or style consideration. Not sure I see either factoring in here, so you could get rid of the additional JSX and just return the ChatEntry component. In general, you'd prefer not adding unnecessary markup.

[marciaga]

Here, you're mutating the original message data that's imported on line 5 above. This was actually causing some tests to fail, in conjunction with a few other points mentioned above and below. For React, you'll want to be cautious about such mutations as it can cause unexpected bugs. The best approach to updating data is actually to create new objects, e.g.

  const toggleLiked = (id) => {
    const messages = messageData.map((message) => {
      if (message.id === id) {
        if (message.liked === false) {
          setHeartTotal((prevHeartTotal) => prevHeartTotal + 1);
        } else {
          setHeartTotal((prevHeartTotal) => prevHeartTotal - 1);
        };

        return {
          ...message,
          liked: !message.liked,
        }

      } else {
        return message;
      }
    });

    setMessageData(messages)
}

But better still would be to do this while also leveraging the approach outlined above of reading the previous data passed to a callback function of the state updating function, e.g.

  const toggleLiked = (id) => {
    setMessageData((prevMessages => {
      return prevMessages.map((message) => {
        if (message.id === id) {
          return {
            ...message,
            liked: !message.liked,
          }
        } else {
          return message;
        }
      });
    }));
  }

and then calculating the heart total separately

  const heartTotal = messageData.filter(m => m.liked).length;