Skip to content

Conversation

@valgidzi
Copy link

TREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What does it mean for code to be asynchronous? Asynchronous code allows multiple things to happen at the same time.
Describe a piece of your code that executes asynchronously. How did this affect the way you structured it? The API calls are code that executes asynchronously, structured with the .then and .catch promises.
What kind of errors might the API give you? How did you choose to handle them? The API could return 404, 400, 204, 500, or Network Error. All errors are displayed at the top of the page. 204 status codes are responses without data, so those needed to be handled differently than the other errors. 400 status codes may also return form errors - those are also displayed to the user.
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'm not sure that I understand what this question is asking. If the trips were stored in an array in my program, sorting them by id would improve the time complexity of searching for a single trip. If the list refers to the API, I don't think that sorting trips by id would matter when requesting information for a single trip.

@tildeee
Copy link

tildeee commented Dec 5, 2018

TREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
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
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

Val, I think your code looks wonderful on this project! I see mastery over functions and closures and callbacks. Well done!

I have a few comments--
A lot of them are about me gushing about your code! I really like your submission for this project.
A lot of comments are about semicolons on my end. I just like throwing a few in there ;) I'll start to have less of those comments and maybe ask you to rely on your javascript linter more.

Well done on this project, I think it's very successful!


const capitalize = (string) => {
return string.replace(/^\w/, c => c.toUpperCase());
}
Copy link

Choose a reason for hiding this comment

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

nice helper method!

if (response.status === 204) {
displayNoContentError(response);
} else {
let callback = response.data.length ? parseTripCollection : parseIndividualTrip
Copy link

Choose a reason for hiding this comment

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

I like this code! It shows the depth of your knowledge about this response: if there is a non-zero length of data (it's an array) then it'll be a trip collection. I'm excited that it was so readable for me!

} else {
let callback = response.data.length ? parseTripCollection : parseIndividualTrip
parseGetResponse(response, callback)
}
Copy link

Choose a reason for hiding this comment

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

nice detail and handling the different responses

displayError(error);
});
};
return buildGetRequest;
Copy link

Choose a reason for hiding this comment

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

I think this is super clever and a great way to work the functions and name them so they are both flexible and specific. Nicely done!


const parseGetResponse = (response, callback) => {
let tripData = response.data
let element = callback === parseTripCollection ? $('#trip-list') : $('#trip-detail-list')
Copy link

Choose a reason for hiding this comment

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

Nice ternary!


const parseGetResponse = (response, callback) => {
let tripData = response.data
let element = callback === parseTripCollection ? $('#trip-list') : $('#trip-detail-list')
Copy link

Choose a reason for hiding this comment

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

Missing semicolons in the above two lines

let tripData = response.data
let element = callback === parseTripCollection ? $('#trip-list') : $('#trip-detail-list')
element.empty();
callback(tripData, element);
Copy link

Choose a reason for hiding this comment

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

This logic allows you to be flexible about exactly which action is happening at this time. Niiice.

};

const parseTripCollection = (tripData, element) => {
reportStatus(`Successfully loaded ${tripData.length} trips.`)
Copy link

Choose a reason for hiding this comment

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

semicolon

}

const parseIndividualTrip = (tripData, element) => {
reportStatus(`Successfully loaded ${tripData.name}.`)
Copy link

Choose a reason for hiding this comment

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

semicolon

element.append(`<h3>${tripData.name}</h3>`);
const tripProperties = ['continent', 'category', 'weeks', 'cost']
tripProperties.forEach((prop) => {
let header = capitalize(prop);
Copy link

Choose a reason for hiding this comment

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

this can be a const. I like this line! very readable about what's going on.

<label for="email">Email</label>
<input type="text" name="email" />
</div>
<input type="submit" name="add-res" value="Submit"/>`)
Copy link

Choose a reason for hiding this comment

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

semicolon. Nicely done on this

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