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

Trek - Emilce - Octos #22

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

Conversation

emilcecarlisa
Copy link

TREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What does it mean for code to be asynchronous? Code is asynchronous when part of the data you're trying to display/use has to come from another server/API and you make a promise to complete to complete the code execution when that data comes back.
Describe a piece of your code that executes asynchronously. How did this affect the way you structured it? It's used to make a reservation and get a list of trips. Functions were based off of those data calls.
What kind of errors might the API give you? How did you choose to handle them? When I tried to get an ID attribute from an http response I was running into an error and was able to peek into the error with error helper methods.
Do you have any recommendations on how we could improve this project for the next cohort?

@droberts-sea
Copy link

TREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene yes
Comprehension questions yes - Regarding asynchronous code, anything that runs at some unspecified time in the future is asynchronous, including everything from AJAX success / failure callbacks to button click handlers.
Functionality
Click a button to list trips yes
Click a trip to see trip details yes
Fill out a form to reserve a spot yes
Errors are reported to the user yes
Styling yes
Under the Hood
Trip data is retrieved using from the API yes
JavaScript is well-organized and easy to read mostly - see inline
HTML is semantic mostly - see inline
Overall Excellent job overall! This code is for the most part well-organized and easy to read, and it's clear the learning goals of this assignment were met. I especially appreciate the care you put into error handling. I've left a couple inline comments for you to review below, but in general I am quite happy with this submission. Keep up the hard work!

<div class="aside">
<section id="details">
</section>
</div>

Choose a reason for hiding this comment

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

Why not use an <aside> here instead of <div class="aside">?

const loadTrips = function loadTrips() {
let tripList = $('#trip-list');

axios.get(allTripsUrl)

Choose a reason for hiding this comment

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

You don’t empty out the element holding your trip list before you load it, so if you click on “show trips” multiple times, the trips get added multiple times

axios.get(singleTripUrl + $(this).data("id"))
.then((response) => {
$('#details').empty().append(`<h2>${response.data.name}</h2><h3>${response.data.continent}</h3><p>${response.data.about}</p><p>Category of trip: ${response.data.category}</p><p>Duration of trip (in weeks): ${response.data.weeks}</p><p>$${response.data.cost}</p>`);
reportStatus('Trips Loaded!');

Choose a reason for hiding this comment

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

Line 57 should be broken up into multiple lines. 80 characters is usually considered the widest a line should be.

Also, it might make sense to break the code to render trip details out into a separate function - as is, this function is doing two things (making the request, and rendering data). See also http://callbackhell.com/

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