Codepath Week 2 feedback - Flixter

[jose-esquivel]

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

Commits: https://github.com/Iamogeee/flixster-starter/commits/main/

[sinap-fb]

This was quite a complex app to build - amazing work!

Code review

• We should avoid early returning in a React component when there are hooks used later on, as this can cause bugs. (more info) https://github.com/Iamogeee/flixster-starter/blob/4d805dd6c593dbe34080b8169992a07853fbd758/src/components/Modal/Modal.jsx#L32-L34
• The <Modal> component appears twice in App.jsx. Sometimes, this duplication is fine, but often we want to try to avoid it since it could lead to future bugs (e.g. somebody adds a prop to one of the but not the other). In this case, it seems like we could move the <Modal> outside of the ternary (e.g. here) so that it only appears once in the code.
• There might be a bug with how pagination is implemented, since pageNumber only ever increases (it's never reset to 1 until the user refreshes the page). If the user clicks "Load more", then clicks the Search button, then goes back to Now Playing, we're now in a state where pageNumber is 2 but we only have the first page of data. This means that if the user clicks Load More, they would fetch the API with page=3 instead of page=2

Minor nitpicks

• Since the project is using CSS files, we should avoid using inline styles when possible, such as here https://github.com/Iamogeee/flixster-starter/blob/4d805dd6c593dbe34080b8169992a07853fbd758/src/components/Header/Header.jsx#L27
• Try to keep prop names and variable names consistent where you can. For example, in App.jsx, I'd rename handleIsSearching to something like handleSearchChange
• We're using an <h2> tag for both the title in the header, as well as the movie title in the (modal). Since heading elements (e.g. <h2>) are hierarchical, we could make the title in the header an <h1>.

Food for thought

• I think the way that the app fetches the movies could be improved, though it would probably be a sizable effort to switch approaches. The current approach is to hold a URL string in state, and fetch it in a useEffect that runs whenever pageNumber or url change. The drawback of this approach is that it makes the logic harder to reason about since the fetch isn't happening directly in event handlers. One pitfall of the useEffect approach is that it will only trigger if the dependencies change. For instance, if we want to do a fetch, we'd call setUrl to do so, but this will only trigger the useEffect if the url state actually changes.
  • Another approach would be to remove the url state and instead fetch stuff directly in each event handler. One drawback of this is probably that it requires much more code than the current approach (though I think the maintainability would improve overall)
• Ideally we avoid wiping the movies state (eg here) so that we can provide a faster UX in some cases. However, this might be tricky with the current approach.

[mylesdomingo]

Great work! There's a lot of moving parts but overrall the commits were pretty clean and easy to follow. A couple of points:

UX:

• In the video, you showed sorting functionality by different types (name, release date, rating, etc). I've noticed for some of these sort types, we don't show the sort key on the card. For example, if I sorted by release date, I'd expect to see the release dates in the card.
• What does "Rating: 0" actually mean? It's important to think about data types and their values. In the screenshot below, we see "Rating: 0" instead of release dates. Does this mean that every user rated their movie as 0 stars? Probably not; likely there were no reviews for that particular movie.
• I'd like to see in the future having the UI adapt and check for these types null cases. Maybe something like showing "No Ratings" instead?

Code Review

• I think the app call to fetch movies can be written a bit better (link). @sinap-fb already focuses on performance but I wanted to point out that the code block itself and talk about promise chaining and async / await.

  const trailerUrl = await fetch(videosUrl)
    .then((response) => response.json())
    .then((response) =>
      response.results.find(
        (video) => video.site === "YouTube" && video.type === "Trailer"
      )
    )
    .then((trailer) => `https://www.youtube.com/embed/${trailer.key}`)
    .catch((error) => {
      console.error("Error fetching movie trailer:", error);
    });

• Promise chains and async / await are two different models of how to handle asynchronous data. If I need to fetch data from the server and it doesn't come instantaneously, I should do one of two things (1) set a promise that the data will return or (2) wait. Generally, in most cases its easier to do (2).

• We can do something like this instead:

// Forces next line of code not to run util data has been fetched completely.
try {
    const data = await fetch(videosUrl);
} catch (e) {
    console.error("Error fetching movie trailer:", error)
}
const videos = data.json();
const trailer =  video.results.find(
    (video) => video.site === "YouTube" && video.type === "Trailer")
);
const trailerUrl = `https://www.youtube.com/embed/${trailer.key}`;

Note how using both promises and async/await are redundant because they achieve the same goal: handling asynchronous data.

This combines 2 things that are generally more complex concepts in JS, but are essential as a web developer interacting with APIs: try/catch blocks and promises + async/await.

I would take a look at these concepts in MDN for more info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Using_promises https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/try...catch

[jose-esquivel]

Echoing what the others said, finishing this app and being able to complete so many stretch features is an amazing feat, You should be very proud about it!

Highlights:

• Great work with separating the work into several commits, this made it easier to follow what you were doing on each one of these.
• I also really like the structure you gave to the project, having each component in a folder with its JSX file and style sheet. This makes it a very easy project to navigate and maintain.
• Code style overall is very good, variable names are clear, logic is easy to follow.

To improve:

• Code: Remember to remove console.log statements from code that is ready to review/deploy
  • https://github.com/Iamogeee/flixster-starter/blob/4d805dd6c593dbe34080b8169992a07853fbd758/src/components/MovieList/MovieList.jsx#L13

• On the UI:

  • I would have liked to see a null-state indicating when the search query got zero results, instead of just displaying the empty screen.
  • Some visual bugs popped up on the search results page:

  

  (See how) the buttons come out of the movie card

  • Remember to pay attention to these kind of issues and be proactive in fixing these bugs. Attention to details like this goes a long way in improving the user's experience.

Overall I'm very happy with the progress you've shown this, excited for the projects to come!