Skip to content

Conversation

@lmarianarp19
Copy link

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone? The model include the application data and business logic
How did the presence of Models and Collections change the way you thought about your app? It helps me to have more understanding over the elements that my applications manage.
How do Backbone Events compare to DOM events? Dom events are triggered when an user makes an action while Backbone events are triggered when the data changes.
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 helps to target certain events and it helps to separate html from js files.
Do you have any recommendations on how we could improve this project for the next cohort? It would be useful to talk about URLRoot and how to modify the url with the item id. Also it would be helpful spend more time talking about how to pass information between methods, like ids

@CheezItMan
Copy link

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene WAAAAY too few commits. Good commit messages
Comprehension questions Check, we'll take your feedback into account.
Organization
Models and collections are defined in separate files Check
Code that relies on the DOM is located in or called by $(document).ready Check
Code follows the Backbone data flow (DOM event -> update model or collection -> Backbone event -> update DOM) Check, well done
Functionality
Display list of trips Check
Display trip details Check
Register for a trip Check
Add a trip Check, but the new trip doesn't show up in the list until the page is refreshed. Instead you can do a tripList.fetch() after a successful save triggering an update event and a re-render.
Sort trips Check
General
Snappy visual feedback for user actions The interface looks good and responds crisply.
API error handling Check, well done
Client-side validation MISSING no client-side validations :(
Overall You hit most of the learning goals, the biggest problem is a lack of client-side validations. For the next project, try to work on that so you can make sure you understand how Backbone validations work.

const inputElement = $(`#reservation-form input[name="${ field }"]`);
const value = inputElement.val();
reservationData[field] = value;
inputElement.val('');

Choose a reason for hiding this comment

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

Right here you're clearing all the field values, including the hidden field where you're hiding the trip id. Instead wait to clear the values until a save is successful. If someone fails validations, the trip Id is being cleared and thus no subsequent reservation will work.

};

// Add a new status message
const reportStatus = function reportStatus(status, message) {

Choose a reason for hiding this comment

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

This function should also clear the container of error messages. Now they never go away.


const Reservation = Backbone.Model.extend({
url: function() {
console.log('https://ada-backtrek-api.herokuapp.com/trips/'+this.trip_id+'/reservations');

Choose a reason for hiding this comment

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

It might make more sense to use urlRoot like so:

urlRoot() {
  return `https://ada-backtrek-api.herokuapp.com/trips/${this.get('trip_id')}/reservations`;
}

It's also preferred style to use interpolation.

const Trip = Backbone.Model.extend({
url: function() {
return `https://ada-backtrek-api.herokuapp.com/trips/` + (this.id || '')
}

Choose a reason for hiding this comment

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

You should be handling client-side validations here and in Reservation.

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