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

Laura Robertson -- Carets #33

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

Conversation

LauraAddams
Copy link

BackTREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What role does the Model play in Backbone? The model good for validations and extra functions you want to create.
How did the presence of Models and Collections change the way you thought about your app? Each component was easier to group. The organization felt much more OOD.
How do Backbone Events compare to DOM events? Backbone events are more related to the data while DOM events are more related to click/type/etc actions.
How did you approach filtering? What was your data flow for this feature? I skipped filtering since it became optional and I wanted to focus on preparing for interviews :)
What do you think of Backbone in comparison to raw JavaScript & jQuery? Backbone, like rails, separates methods/functions/data making it clear where to put individual elements and making the overall code easier to read.
Do you have any recommendations on how we could improve this project for the next cohort? The timing was off. I was able to get the baseline done, but not to the level I would have liked.

@CheezItMan
Copy link

BackTREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene More granular commits would be better, but good commit messages.
Comprehension questions Model also provide communication to the API. Understood about the timing on Backbone.
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
Functionality
Display list of trips Check
Display trip details Check
Register for a trip No validation for registration. I can make an email without an @ for example
Add a trip Check, including validation, but I would suggest putting the validations in the modal, so the user can immediately fix them.
Sort trips Check, well done
General
Snappy visual feedback for user actions I like the modal. Good work on it. The site is responsive with simple and effective styling.
API error handling Well done
Client-side validation Client-side validation done on the trips. Nice work
Overall Well done, you hit the primary requirements. I made some small minor suggestions in your code. Nice work.

});

// ------------- Reserve a Trip --------------
$('body').on('submit', '.book-trip', function form(e) {

Choose a reason for hiding this comment

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

validations??

<form class='add-trip-form'>
<h4>Add a trip</h4>
<input type='text' name='name' placeholder='Name' />
<input type='text' name='continent' placeholder='Continent' />

Choose a reason for hiding this comment

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

A select element would make a better choice, not giving the user an option for an invalid continent.

<h4>Add a trip</h4>
<input type='text' name='name' placeholder='Name' />
<input type='text' name='continent' placeholder='Continent' />
<input type='text' name='about' placeholder='About' />

Choose a reason for hiding this comment

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

A textarea would make a better choice.

});

$(document).on('click', function() {
const modal = document.getElementById('add-trip');

Choose a reason for hiding this comment

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

Avoid mixing JavaScript and jQuery selections, use one or the other, in this case I'd suggest jQuery.

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