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

Lauren Cardella -- Carets #42

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

Conversation

enigmagnetic
Copy link

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone? The Model plays largely the same role as models in Rails, that is, to hold data and business logic for related objects.
How did the presence of Models and Collections change the way you thought about your app? I'm not sure that it did. Maybe because I thought of objects like Trips as being models before. It's definitely a clearer way of organizing the code according to the MVC convention.
How do Backbone Events compare to DOM events? Backbone events are triggered by data changes and DOM events are triggered by user actions.
How did you approach filtering? What was your data flow for this feature? I have not yet approached filtering but I will do so this week and resubmit.
What do you think of Backbone in comparison to raw JavaScript & jQuery? It's nice to have some built-in methods that handle things like RESTful conventions and other convenient shorthand ways of doing things (i.e. forEach loops).
Do you have any recommendations on how we could improve this project for the next cohort?

…. Add basic HTML structure. Build loadTrips() function to retrieve trip collection. Build Trip model and TripList Collection.
…he about attribute to be recognized within the template.
… Add client-side validations for new trip. Set up error handling.
@CheezItMan
Copy link

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene More granular commits would be better.
Comprehension questions Check, backbone model also communicate with the API.
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) Attempts are made, however addinga trip doesn't cause the tripList to refresh from the API.
Functionality
Display list of trips Check
Display trip details Check
Register for a trip Check, but missing validations.
Add a trip Check, but it's not adding the new trip to the list
Sort trips It sorts, but subsequent clicks don't reverse the order.
General
Snappy visual feedback for user actions nice animations for the register for a trip form. Good UI overall
API error handling Reporting of errors to the user however the Add a trip errors are behind the modal and reporting of errors in registration simply say it failed, not why.
Client-side validation Similar to API Errors, and the registering form performs no validations.
Overall You hit most of the requirements, and it's a good start on the project, given your illness last week. Let me know if there are things you want me to look at in the future.

src/app.js Outdated
updateStatusMessageFrom(trip.validationError);
}

tripList.fetch();

Choose a reason for hiding this comment

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

This fetch will be run before the trip finishes saving and thus the new trip may not show up in the tripList.


const successfulSave = function(trip, response) {
updateStatusMessagesWith(`${trip.get('name')} added!`)
clearForm();

Choose a reason for hiding this comment

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

This is where you should have the tripList refresh, or fetch the trip's name and manually add it to the list.

import Backbone from 'backbone';

const Trip = Backbone.Model.extend({
url: function() {

Choose a reason for hiding this comment

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

As I said earlier, you could also replace this url function with:

urlRoot: https://ada-backtrek-api.herokuapp.com/trips/

urlRoot is the endpoint that the API begins and Backbone can infer RESTful routes from there.

src/app.js Outdated
$('#reservation').hide();
clearForm();
}).fail(() => {
$('#message').html('<p>Reservation failed to save.</p>');

Choose a reason for hiding this comment

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

You really should have some client-side validations, just if statements checking to make sure the form fields have valid values.

src/app.js Outdated
tripListElement.append(tripTemplate(trip.attributes));
});

$('th.sort').removeClass('current-sort-field');

Choose a reason for hiding this comment

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

What about if it's already sorted by this field? How could you then reverse the order?

@enigmagnetic
Copy link
Author

Hi Chris,

I added filtering capabilities and made the suggested edits. The filtering doesn't quite work the way it should--if I sort a filtered list it removes the filtering and shows the entire list again.

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