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 - Kee & Iuliia - Video Store Consumer #15

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

Conversation

julalam
Copy link

@julalam julalam commented Dec 21, 2017

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 both learned about Backbone and gained a deeper understanding of how views and models worked.
What was one area of Backbone you gained more clarity on during this assignment? We learned more about connecting views together, how to better use the event bus, and initializing models with attributes.
Describe how you solved the problem of having lists of movies that look mostly the same but have different content We brute-forced it. We used two instance variables to save our rental library and the search results list separately in a single view.
Describe how you handled rentals with Backbone? We did not handle rentals.
Describe a DOM event your application handled When we searched for a query, we would render the movie list by appending movie views.
Describe a custom event your application handled We used the event bus instead of custom events.
Do you have any recommendations on how we could improve this project for the next cohort? More time would be great!

@julalam julalam changed the title Pipes - Kee & Iuliia - Video Store Pipes - Kee & Iuliia - Video Store Consumer Dec 21, 2017
@droberts-sea
Copy link

Video Store Consumer

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene - pair contribution yes
Comprehension questions yes - Vocab nitpick: this is not quite what brute force means. Brute force is a method where you try all possible combinations of something until you find one that works. The name comes from brute force code cracking, where you attempt to decrypt an encrypted message with every possible key until a reasonable message pops out. As an example, a brute-force sorting algorithm would generate all possible permutations of a list, checking them one by one until it found one that was in sorted order.

I don't think the term really applies to this project. Instead, I would probably say something like "instead of building a complex design, we kept it simple and maintained two separate lists of movies".
General
Search functionality yes
Rental library listing functionality yes
Add to rental library functionality tes
Underscore Templates yes
Overall Look & feel mostly good - it seems like your movie synopsis overlaps with the poster image sometimes
Optionals
Overall Good work overall! The way events travel through your application seems well-designed.


if (this.mode == "search") {
this.collection.forEach((movie) => {
const movieView = new MovieView({

Choose a reason for hiding this comment

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

We may have talked about this in person, but I'm not a big fan of the names model and collection for these instance variables. It would be very easy to get confused about which one is the rental library and which ones are movieDB results. Could you give them more descriptive names?

addtoLibrary() {
let saved = this.model.save();
if (saved) {
this.collection.add(this.model);

Choose a reason for hiding this comment

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

You should probably handle errors if the movie does not save.

let found = this.collection.where({overview: this.model.get('overview')}).length > 0;
this.model.set('inInventory', found);

this.bus.trigger('addToCollection', this.model);

Choose a reason for hiding this comment

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

I'm not sure addToCollection is a good name for this event, since you haven't added anything to a collection. Perhaps something like selectMovie would be more mnemonic.

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