-
Notifications
You must be signed in to change notification settings - Fork 42
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 - Tamira - Trek #27
base: master
Are you sure you want to change the base?
Conversation
…at this is not such a nested mess
… less conviluted:
TREKWhat We're Looking For
Great job overall! In my estimation there are two difficult things in JavaScript we've never had to do in Ruby/Rails:
Based on this submission I'd say you have a good handle on both. These topics will get substantially more complex as we continue to build bigger apps with more moving parts, and having strong organization in your JavaScript code will be even more important. Keep up the focus and hard work! |
// create a click event for the button to book a trip | ||
$(`#trip${id}`).click((event) => { | ||
// stops the tripHTML click event from running | ||
event.stopPropagation(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a callback inside a callback inside a callback inside a callback inside a callback! To me, this is a good indicator that you should break some of these out into separate named functions, defined at the top of the file. Each jQuery event handler and AJAX success or failure callback should get its own function.
See also callback hell
let form = $( | ||
`<div id="book"> | ||
<form action="${action}" method="post" id="book${id}"> | ||
<label for="name">Name:</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't close this <div>
// create click event for each trip name | ||
tripHTML.on('click', '.details', (event) => { | ||
let tripId = $(event.target).attr('id'); | ||
getTripDetails(tripId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good effort at refactoring (certainly better than the original), but you're still in a triply nested callback here.
TREK
Congratulations! You're submitting your assignment!
Comprehension Questions