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

Melissa : Edges : Trek #36

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

Conversation

melicious-dish
Copy link

TREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What does it mean for code to be asynchronous? Code that is asynchronous means that you can do many things at once, like sending a request while interacting with the web page
Describe a piece of your code that executes asynchronously. How did this affect the way you structured it? the axios.get response was sending a request while the trip list loaded.I put the load trips in the document.ready so that would load before the page
What kind of errors might the API give you? How did you choose to handle them? The errors that came back were that fields couldn't be empty. My code was written correctly (in the wrong function), so some rearrangement - making a new function helped
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. Yes it would as the id was also used to create a reservation

@tildeee
Copy link

tildeee commented Dec 11, 2018

TREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene x
Comprehension questions x, ordering trips by ID shouldn't be advantageous by your reasoning that the ID is used to create a reservation
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

Nicely done on this project, Melissa! I'm really happy with how the code looks-- incredibly clean, readable, logical, and practical! It's a great project submission.

I have a few nuanced comments on how you approached the event delegation, but otherwise I'm really pleased with the code. Really great work!

reportStatus(`Successfully loaded ${response.data.length} trips`);
// sort
response.data.forEach((trip) => {
tripList.append(`<li><a href='#' class="trip_id" id=${trip.id}>${trip.name}</li>`);
Copy link

Choose a reason for hiding this comment

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

You should probably not include the `href='#' bit, otherwise it changes the URL of the browser

also instead of setting trip.id to be the value of id, you could probably set it to an attribute like data-id instead


$('body').on('click', '.trip_id', function(event){
loadTripDetails(event.target.id);
});
Copy link

Choose a reason for hiding this comment

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

nice use of Event Delegation. I personally would've preferred if the selectors were more specific (instead of selecting on body you waiting until showing the #trip-list and selecting that) (or also, changing the name of the class of trip_id since that's not really accurate for what those elements are), but it works all the same!

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