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

Carets -- Guillermina #29

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

Carets -- Guillermina #29

wants to merge 13 commits into from

Conversation

murog
Copy link

@murog murog commented Dec 4, 2017

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone?
How did the presence of Models and Collections change the way you thought about your app?
How do Backbone Events compare to DOM events?
How did you approach filtering? What was your data flow for this feature?
What do you think of Backbone in comparison to raw JavaScript & jQuery?
Do you have any recommendations on how we could improve this project for the next cohort?

@Hamled
Copy link

Hamled commented Dec 14, 2017

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene
Comprehension questions No. You should try to answer these questions.
Organization
Models and collections are defined in separate files
Code that relies on the DOM is located in or called by $(document).ready
Code follows the Backbone data flow (DOM event -> update model or collection -> Backbone event -> update DOM) Mostly. There is one case where we are doing this: after calling tripList.fetch(); in $(document).ready() we do get an update event which is results in the render. Other than that all of the code to update the DOM is running in handlers for jQuery events.
Functionality
Display list of trips
Display trip details
Register for a trip No. This is not currently implemented.
Add a trip No. This is not currently implemented.
Sort trips No. This is not currently implemented.
General
Snappy visual feedback for user actions
API error handling No. We're not handling possible API errors when fetching the trip list or details for an individual trip.
Client-side validation No. This is not implemented because adding trips and reservations is not implemented.
Overall I really like the design for this site!

const selectedTrip = tripList.findWhere({id: selectedId});
console.log(url);
$('#trip-details').show();
$.get(url, function(response) {
Copy link

Choose a reason for hiding this comment

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

We should try to use Backbone's fetch function for models here instead of directly using jQuery's ajax functions.

let tripInfoTemplate;
const render = function(tripList) {
console.log('in render');
$('#trip-list').append('<tr><th>ID</th><th>CONTINENT</th><th>COST</th><th>CATEGORY</th></tr>');
Copy link

Choose a reason for hiding this comment

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

Before we start filling in the contents of the trip list we should clear it out by calling $('#trip-list').empty();.

This allows the render function to be idempotent, so that if our application has to call it multiple times (such as when adding or removing a trip), it will still do the correct thing.

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