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 - Kate Evans-Spitzer - BackTREK #40

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

Conversation

Guribot
Copy link

@Guribot Guribot commented Dec 4, 2017

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone? Managing data & logic regarding the data
How did the presence of Models and Collections change the way you thought about your app? Most significantly, it helped me standardize the way I interacted with the data - when I consistently thought of the trip list as a Collection of Trip Objects, I had a more organized approach to working with it.
How do Backbone Events compare to DOM events? They are very much the same, with DOM events referring to user-triggered front-end events and Backbone events being triggered by the program/back-end
How did you approach filtering? What was your data flow for this feature? I added a filterBy function to the TripList Collection, taking advantage of Backbone's implementation of the JavaScript Array#filter function. It returns a new, filtered instance of the original collection so as to avoid modifying the original list (and theoretically making the filters chainable) while still passing through attributes like comparator.
What do you think of Backbone in comparison to raw JavaScript & jQuery? I'm a fan :) I'd like to see how the views work since it does seem like there is a missing puzzle piece right now. It feels weird having so many underscore templates sitting at the bottom of the HTML.
Do you have any recommendations on how we could improve this project for the next cohort? Nope - timing is a bit weird, it would be nice to be able to work on during interview downtime, but I recognize the balance of that is difficult since it would be frustrating to HAVE to work on it during interview week.

@CheezItMan
Copy link

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good number of commits and good commit messages.
Comprehension questions Check, I'm glad you're a fan!
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, except for sort events.
Functionality
Display list of trips Check
Display trip details Check
Register for a trip Check
Add a trip Check
Sort trips Check, but see my note on listening for sort events.
General
Snappy visual feedback for user actions The styling is really really good. It provides good validation feedback. Good use of modals!
API error handling Check, nicely done
Client-side validation Check
Overall Well done, you hit all the primary requirements. See my in-code notes, but this was good work!

});
['name', 'continent', 'category', 'weeks', 'cost'].forEach((field) => {
if (attr[field] === '') {
errors[field] = "can't be blank";

Choose a reason for hiding this comment

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

I would suggest also adding an if statement like

if (errors[field]) {
  errors[field].push('can't be blank');
} else {
  errors[field] = ['can't be blank'];
}

That way you can have multiple errors for a field.

if (parseFloat(attr[field]) === NaN) {
errors[field] = "must be a number";
}
if (!parseFloat(attr[field]) > 0) {

Choose a reason for hiding this comment

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

No free trips?

if (object.isValid()) {
object.save({}, {
success: (response) => {
formSuccess(type, form)

Choose a reason for hiding this comment

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

Note, for trips, this won't allow a newly created trip to show up in a displayed list. The user would have to refresh the browser to see it.

On a successful trip save, you should execute a tripList.fetch();. That would trigger render because it's listening for an update event.

I do however really like how you abstracted out the saveIfValid function so it works for all your models. Nice!

$('#intro-button').on('click', (e) => {
$('#intro-button').hide(200);
$('#loading').show(500);
tripList.fetch({

Choose a reason for hiding this comment

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

You should also have an error callback, just in case the API is down to alert the user.

let field = targetElement[0].id;
tripList.comparator = field;
targetElement.addClass('current-sort');
tripList.sort();

Choose a reason for hiding this comment

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

It's better stylistically to have render listening for a sort event.

So adding tripList.on('sort', render);

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