Skip to content

Conversation

@pushpaagr
Copy link

@pushpaagr pushpaagr commented Nov 26, 2018

TREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What does it mean for code to be asynchronous? Asynchronous means that you are able to do many things at the same time, for example you can interact with the webpage while its waiting for a request, and it won't freeze. It also allows you to send multiple requests at a time.
Describe a piece of your code that executes asynchronously. How did this affect the way you structured it? When a user clicks on a single trip, it not only loads the trip detail but it also populates the form for a trip reservation. I had to structure my code in a way that it both of these actions would happen when a user clicked on a single trip.
What kind of errors might the API give you? How did you choose to handle them? The API does not allow a name or email to be empty if you are trying to create a reservation. I handled this error by displaying the message in a user friendly way that shows the message of 'name/email can't be blank'
Suppose you needed to routinely find a specific Trip in the list by it's ID field. Would it be advantageous to keep the list in order by the id field? Explain why or why not. I don't think it would make a difference if it was in order or not since we already have the id that we need in order to look for the trip. Right now the order of my list does not matter, when a user click on a trip, its the click that triggers the trip id information, and the click is the one that is getting the id for that particular trip.

@tildeee
Copy link

tildeee commented Dec 11, 2018

TREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x, it would be better to have smaller commits with commit messages that describe the code changes of what happened, and not "wave 2" etc...
Comprehension questions x
Functionality
Click a button to load and view a list of trips x
Click a trip to load and view trip details x
Fill out a form to reserve a spot x, bug about the form detailed below
Errors are reported to the user x
CSS is DRY and attractive, uses CSS Grid, flexbox, and/or Bootstrap x
Under the Hood
Trip data is retrieved using from the API x
JavaScript is well-organized and easy to read x
HTML is semantic x
Overall

Good work, Pushpa. Your project works and meets the requirements and works pretty well! There are a few concerning choices about some ways you approached the problems... While your code does work, I have concerns about the use of global variables. Also, your form disappears when I look at every-other trip...! Ahhh!!

I've left a few comments. This being said, you did pretty okay on this.

@@ -0,0 +1,129 @@
//wave 1
const URL = 'https://trektravel.herokuapp.com/trips';
let currentTripID = 0;
Copy link

Choose a reason for hiding this comment

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

You're putting the currentTripID in this global scope in order to store what the current trip is... You should consider different approaches to get the value of currentTripID. Could you use a closure or read from the HTML?

loadTripDetails(`${trip.id}`)
});
trips.on("click", function() {
$("#addForm").toggle()
Copy link

Choose a reason for hiding this comment

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

using toggle() here produces a bug: When I click to view other trips, then the form hides, until I click again. It makes it unusable to reserve more than one trip!


//wave 2

const URL1 = 'https://trektravel.herokuapp.com/trips/';
Copy link

Choose a reason for hiding this comment

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

What's the difference between this variable and the above variable URL? They're not different! Why can't you reuse it? Even if they were different, could you use a different variable name besides URL1 that is more descriptive? Also, we like declaring global variables at the top of the file, otherwise they get lost.

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