Codepath Week 3 feedback - Kudos Board

[jose-esquivel]

Thread for providing feedback for Ogenna's third week project.

Commits: https://github.com/Iamogeee/KUDOS-BOARD/commits/main/

[sinap-fb]

Nice work! The server code was well organized and easy to follow! One thing I noticed is that the deployed site doesn't seem to work for. It seems like the fetches are hitting localhost:3000 instead of the production backend.

However, I didn't see any mention of localhost in the code, so I assume this was fixed but the fix hasn't been deployed yet?

Code review:

• It seems like CardList.jsx renders not only the list of cards, but also the "Create New Card" button (and the Creation dialog). I think it would make sense (if feasible) to split these up into 2 separate components - a CardList and a "NewCardButton" (or whatever).
• We refetch the list of cards after upvoting a card. The benefit of this approach is the simplicity. However, the drawback is that we need to refetch all of the cards, which is a slower UX. An alternative would be to update the cards state such that we only modify the array item that corresponds to the card we just updated. It might look something like this:
  • setCards(prevCards => prevCards.map(prevCard => prevCard.id === id ? card : prevCard)) (where card comes from the response to the PUT request)

Things to consider for the future:

• I think more comments could be helpful, if you have the time in future projects. For instance, this endpoint seems to upvote a card, but that wasn't immediately clear to me.
• I'm guessing this is out of the scope of the assignment (since it's a pretty advanced topic), but there seems to be a "race condition" in the code. Throughout App.jsx, we're updating React state after the network request completes. The issue with this is that, if a user takes some action that results in multiple fetches issued in quick succession (e.g. upvote a card and then quickly switch to another board), the response to the first fetch might be received after the response to the second fetch. In this case, we'd clobber the React state that we just set from the second fetch. In practice, this probably won't happen, but if the app was being used by a lot of users, they might eventually hit a bug caused by this issue. So something to keep in mind, but I definitely don't expect a newcomer to handle this edge-case.

Super nitpicks:

• try to keep variable naming style consistent. So instead of img_url maybe imgUrl to keep the naming style ("camel case") consistent throughout the app.

[jose-esquivel]

Great work again, Ogenna!

I'm happy to see how you continue to develop your knowledge and coding style through these projects.

Highlights

• Great code organization on both the frontend and backend parts of the project
• Very good use of flex-box on the website's layouts
• The project displays good understanding of React hooks like useState, useEffect, etc.
• Backend endpoints are well laid out, respecting RESTFul practices

Code feedback

• https://github.com/Iamogeee/KUDOS-BOARD/blob/f83368c8afcceadaef3dbeb0658b2aeebb981c08/frontend/src/CreateForm/CreateForm.jsx#L4
  • It would be a good idea to split this form into two different components, CreateCardForm, CreateBoardForm. It would have been cleaner to have the parent component perform the logic to show either of this components. Smaller component files are usually more readable and easier to maintain.
• https://github.com/Iamogeee/KUDOS-BOARD/blob/f83368c8afcceadaef3dbeb0658b2aeebb981c08/frontend/src/Card/Card.jsx#L14
  • I would have liked to see a refetch of cards or performing a client update on the card data and navigating to the main page using the app router, instead of reloading the whole website to remove the card. This can lead to a smoother user experience by avoiding page reloads.

Commit style

• The commits were large this time around, with many changes on each of them. Please consider making them smaller for your next project.
• Try to keep commented code in your commit history to a minimum. Commented code or console.logs should not make it to pull-requests.

UI/UX polish feedback

• Consider removing or adding disabled states to filter buttons, in the case where there are no boards of that category to be displayed. It can be a bad user experience when we have a button that does nothing when pressed, it's better to indicate when its function is not available by using a disabled state.
• When displaying media like images, gifs or videos, it's good to be mindful of respecting the original aspect ratio of the media to be displayed. In this case we are displaying all of the gifs in a vertical aspect ratio and stretching all of the gifs to conform to it. Stretching like this can make images look distorted or strange.

[mylesdomingo]

Hey, a bit of a late review, but this project looks good! I have a couple points of feedback:

Highlights

1. Nice work on handling various promises and improving on async/await calls from last week. Looks like you got the hang of it.

   Code Quality

2. When rendering multiple elements in a list, its generally good to use a map. Instead of:

   <Button name="All" />
   <Button name="Recent" />
   <Button name="Celebration" />
   <Button name="Thank You" />
   <Button name="Inspiration" />

   I'd do something like:

   const buttons = {...}
   return buttons.map(({name}) => <Button name={name} />)

It's a bit silly now, but when you have multiple of the same props and stylings and more than 5 objects, it saves a lot of time/redundancy.

2. I think handleDisplayBoard and handleDeleteBoard could have been handled more efficiently. For context, handleDisplayBoard makes a call to our backend to fetch data. That call could be computationally expensive.

We also have a local state const [boards, setBoards] = useState([]); that tracks the boards currently on display in the screen. When we delete a board via handleDeleteBoard. We call handleDisplayBoard and make that fetch all over again -- in cases where network fetching is expensive, so we want to avoid doing so!

Instead I would have like to have seen treating the local state as a "cache". In handleDeleteBoard, we write to our db. We know what our local state should look like, so we can update this as well directly. We do not call handleDisplayBoard again. We only should fetch maybe once per refresh. This would look something like:

  async function handleDisplayBoard() {
    const response = await fetch("http://localhost:3000/boards", {
      method: "GET",
      headers: {
        "Content-Type": "application/json",
      },
    });
    const data = await response.json();
    setBoards(data);
  }

  useEffect(() => {
    handleDisplayBoard();
  });

  async function handleDeleteBoard(id) {
    try {
      const response = await fetch(`http://localhost:3000/boards/${id}`, {
        method: "DELETE",
        headers: {
          "Content-Type": "application/json",
        },
      });
      if (response.ok) {
         setBoards(boards.filter(boards => board.id != id)) 
      }
    } catch (err) {
      console.log(err);
    }
  }

Notice how we update our local state without making a roundabout fetch from our db. There's more optimizations to be done with caching, maybe turning the list into an object for O(1) indexing. But the principles remains the same: network calls are performance intensive, optimizing these calls increases app responsivity!

You probabaly already know this, but here's a primer on big-O notation (just in case). It's applicable for front-end too! https://www.shecodes.io/athena/52559-understanding-big-o-notation-in-javascript#:~:text=Big%20O%20notation%20is%20a,the%20more%20time%20it%20takes.