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 - Irene - BackTREK #22

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

Pipes - Irene - BackTREK #22

wants to merge 6 commits into from

Conversation

idevera
Copy link

@idevera idevera commented Dec 4, 2017

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone? The model is similar to models(classes) in Rails. The models validate data when inputs of a new instance is to be saved, it can create default values of instantiated object, and you can create custom methods for instantiated objects
How did the presence of Models and Collections change the way you thought about your app? If your app will only be using collections, you can use collections to get data of a single object in your instantiated collection. Both Models and Collections allow us to interact with the requested API and use the data as objects within our application
How do Backbone Events compare to DOM events? Backbone events listen to changes with Backbone Models and Collections ie. when a new Trip is created and the new trip needs to run an isValid method. This is different than DOM events where objects or elements are changed
How did you approach filtering? What was your data flow for this feature? I filtered using HTML and javascript, not the backbone model
What do you think of Backbone in comparison to raw JavaScript & jQuery? Allow us to separate our classes from most of our UI and Business logic
Do you have any recommendations on how we could improve this project for the next cohort? Move filtering to being optional

@CheezItMan
Copy link

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Waaaay too few commits. Also put commit messages in describing functionality added not waves.
Comprehension questions Check, model also handle communication with the API. Also Backbone events don't generally involve knowing when validation is supposed to be run, instead they are typically run when data in a model or collection changes. Note you can see an example of a filtered collection in the lecture material.
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, see an extremely minor note about where to add the update event listener.
Functionality
Display list of trips Check
Display trip details Check
Register for a trip Check, but see my note about your code clearing form field values.
Add a trip Check
Sort trips Check
General
Snappy visual feedback for user actions Minor styling note, trip names etc can overlap with other content on the displayed table.
API error handling Check, well done.
Client-side validation Check, see my notes for additional validations you could have written.
Overall Small note, you've got an error in the console coming from foundation.js which you included via a tag in the html. See my notes on it. You hit the requirements with minor bugs. Nice work!

tripList.fetch();

// Listen and register. Once an update has been heard, render all the collection into the template
tripList.on('update', renderList);

Choose a reason for hiding this comment

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

Minor note: It's theoretically possible (1 in a trillion chance) that the fetch could finish and return before this line being executed. In which case renderList would not be executed. It's more proper to establish your listeners and then run fetch.

<script src="app.bundle.js" type="text/javascript"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/foundation/6.4.3/js/foundation.js"></script>

Choose a reason for hiding this comment

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

By adding foundation here you're getting an error because jQuery isn't defined in global scope. to get rid of it you'd need to link in jQuery via a script tag, or use the foundation-sites npm package.

const Reservation = Backbone.Model.extend({
baseURL: 'https://ada-backtrek-api.herokuapp.com/trips',
url: function() {
return this.baseURL + '/' + this.attributes.tripId + '/' + 'reservations';

Choose a reason for hiding this comment

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

You could also specify a urlRoot function.

urlRoot() {
  return `${this.baseURL}/${this.get('tripId')/+reservations`;
},

Note the string interpolation.

},

validate: function(attributes) {
const errors = {};

Choose a reason for hiding this comment

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

Good, but I'd also suggest making sure the email has an @ in it.

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

Choose a reason for hiding this comment

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

I would also check for a numeric value of weeks and cost and make sure they are non-negative.


reservation[field] = value;

inputElement.val('');

Choose a reason for hiding this comment

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

I suggest only clearing the input values on a successful save. Otherwise you're clearing the tripId and there's no way for a user to enter that.

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