Skip to content

Instantly share code, notes, and snippets.

@rwilliams659
Last active September 3, 2020 17:34
Show Gist options
  • Save rwilliams659/9bbac42cf2906cc0a36be440945fb26b to your computer and use it in GitHub Desktop.
Save rwilliams659/9bbac42cf2906cc0a36be440945fb26b to your computer and use it in GitHub Desktop.
Mod 3 Favoriting Code Review for Linus

Mod 3 Favoriting Code Review for Linus

Specification Adherence: 2

On the homepage, the favoriting works well - it's easy to toggle between favoriting and un-favoriting. The Favorites page also displays as expected. It might have been nice to actually include the Favorites page in the nav bar, as what the "filter" button does might not be clear to a user not familiar with the project specs.

I don't see the option to favorite a movie on the actual Details pages, so it seems that specific feature is missing. Additionally, the heart icons are kind of difficult to see on the homepage; I would suggest making them a bit larger for improved UX.

Project Professionalism: 3

Nice job on creating small, atomic commits. It also looks like the project board was used throughout and is very detailed. It's difficult to comment on this area further without considering the project as a whole!

React Architecture: 3.5

Nice job overall placing the functions that toggleFavorites and trigger the postFavorites & findFavorites apiCalls in the App component. This makes a lot of sense since the favorites array prop lives in App. I also really like that filterFavs was a function living in App and just the favorite movies were passed down as a prop in the Card Section component when on the Favorites page. Again, makes a lot of sense to have all of that happening in App. Nice job with error messages for unsuccessful favoriting. In your findFavorites function, though, the error message should probably be modified to something like "Error retrieving favorite movies" instead of "posting".

One small additional note: I see that in the constructor function of your App component, the API call functions were added as properties; I think you could have foregone this step and just called the functions by their names as imported from './APICalls' e.g. just postFavorite().

Testing: 1.75

Looking at the App test, I'm not seeing acceptance tests for favoriting/unfavoriting and for viewing the Favorites page. I also don't see tests in the MovieCard test file for testing that the favorites icon also displays in the UI, or that the toggleFavorites function is called when the favorites icon is clicked. Unless I'm missing something or these tests were added on a different branch, it seems there is a gap in testing for this feature.

Routing: 3

Routing to the Favorites page in the App render method works well. My only suggestion would be to add the Favorites page to the nav bar in the header with a tag instead of via that filter button in the CardSection UI for improved UX, but the implementation of Router works both ways.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment