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

angela - backtrek - pipes #32

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

Conversation

awilson2017
Copy link

@awilson2017 awilson2017 commented Dec 4, 2017

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone? models takes care of the business logic, validations, etc. just like in Rails.
How did the presence of Models and Collections change the way you thought about your app? They made me think about how a model instance relates to a collection. Like when rendering a trip's details from the list of all the trips.
How do Backbone Events compare to DOM events? Backbone events address data, where as DOM events address elements in the DOM, html tags
How did you approach filtering? What was your data flow for this feature? NA
What do you think of Backbone in comparison to raw JavaScript & jQuery? It is fine. Having a frame work was nice. Side note, I was not about to get the reservation functionality completely down. Where I got stuck was with POSTing to the API. Everything to that point worked fine.
Do you have any recommendations on how we could improve this project for the next cohort? ?

@CheezItMan
Copy link

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Not enough commits, try to make things more granular, also messages like: commit uncommitted files, while understandable aren't very helpful.
Comprehension questions Check
Organization
Models and collections are defined in separate files Check, great!
Code that relies on the DOM is located in or called by $(document).ready Well done
Code follows the Backbone data flow (DOM event -> update model or collection -> Backbone event -> update DOM)
Functionality
Display list of trips Check
Display trip details Check
Register for a trip It works if I create a reservation correctly the 1st time. If there are validation errors (great work with those btw), then it doesn't get the tripId field.
Add a trip Check, but validation error display behind the modal which could be tricky to see. Consider placing them next to the input fields. It's a better UX
Sort trips It sorts, but doesn't change the highlighted column correctly. Ooops.
General
Snappy visual feedback for user actions I like the modal for adding a trip and the UI was simple, but worked. It would be good to have the trip details and trip list on the screen simultaneously.
API error handling Good display of API errors
Client-side validation Great client-side validations, one note you say weeks instead of email for reservation.
Overall Pretty good, you hit all the requirements with minor bugs. See my in-code notes about how to address them. Nice work.

}

if (!attributes.email) {
errors.weeks = ['cannot be blank'];

Choose a reason for hiding this comment

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

Weeks for the reservation?

tripData[field] = value;
}

$inputElement.val('');

Choose a reason for hiding this comment

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

Consider this scenario:

1). I click on a trip, great the form shows up.
2). forget to put in my email address before hitting submit, I get a validation error message, great
3). I notice the fields have their values blanked, also ok as I can reenter them (just annoying), but the hidden tripId field is also blanked now the submission fails.

A better time to do this is after a successful save, just make a helper to blank the form and call it after a successful save.

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.

2 participants