Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pipes - Eva and Rebecca - VideoStore #23

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

rbergena
Copy link

@rbergena rbergena commented Jan 3, 2018

Video Store Consumer

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
For each person in your pair, describe something you learned from your partner during this project We learned how to make a modal and how to make the images populate correctly.
What was one area of Backbone you gained more clarity on during this assignment? We gained more clarity on making API requests through backbone.
Describe how you solved the problem of having lists of movies that look mostly the same but have different content We're not sure what this means, but we made sure that if two movies had the same title you could still see the different images for the movies.
Describe how you handled rentals with Backbone? We did not do the optionals.
Describe a DOM event your application handled We made an event handler for when someone clicked on the add movie button.
Describe a custom event your application handled We had a custom event to search within the current set of movies in the video store.
Do you have any recommendations on how we could improve this project for the next cohort? No

@Hamled
Copy link

Hamled commented Jan 5, 2018

Video Store Consumer

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene - pair contribution
Comprehension questions
General
Search functionality Mostly. There's a missing parameter in the searchApi function in SearchListView which is preventing the search feature from working correctly. After adding that everything seemed to work fine, though. See my comment for details.
Rental library listing functionality
Add to rental library functionality
Underscore Templates
Overall Look & feel
Optionals
Overall I think the design of your site is great!

searchApi() {
//this is an ugly way to reset the URL. See if we can fix it
this.clearMessages('#search-error-message');
event.preventDefault();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're trying to prevent the default browser behavior here by calling event.preventDefault(), however the event variable is not defined in this function. We need to add it as a parameter:

searchApi(event) {
  // ...
  event.preventDefault();
}

},

searchMovies() {
event.preventDefault();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here, we need to define the function as searchMovies(event).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants