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

Jamila Octos C9 #29

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

Jamila Octos C9 #29

wants to merge 7 commits into from

Conversation

Jcornick21
Copy link

TREK

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
What does it mean for code to be asynchronous? When code is asynchronous it does not necessarily execute in order or right away.
Describe a piece of your code that executes asynchronously. How did this affect the way you structured it? creating a trip happens asynchronously I structured it so that one click event gives me the ability to have another click event inside it.
What kind of errors might the API give you? How did you choose to handle them? it might say that the particular thing called for does not exist . I handled it by catching the errors and them printing them out
Do you have any recommendations on how we could improve this project for the next cohort?

.then((response) => {
reportStatus(`Successfully loaded ${response.data.name} trip`);
console.log(response);
const result = response.data

Choose a reason for hiding this comment

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

It seems like on a simple variable assignment like this, you have a tendency to miss the semi-colon. Watch out for this!

// console.log(reservation);
// console.log(trip);

const reservationData = {

Choose a reason for hiding this comment

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

Good

</section>

<aside id="right-aside">
<section id="hide">

Choose a reason for hiding this comment

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

I would use a more semantic id for this that would better indicate what data this contains, even if the ultimate goal is to hide it

$('form').submit( function(event) {
event.preventDefault();
const reservation = this
console.log(trip);

Choose a reason for hiding this comment

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

It might be worthwhile to pull out only trip.id here because neither the tripDetails or createReservation functions are using anything other than the ID

const reservation = this
console.log(trip);

createReservation(reservation, trip);

Choose a reason for hiding this comment

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

I like the way that you used different functions here to handle the tripDetails and the createReservation logic. This makes the nested event handlers (aka using closures) a bit easier to read.

@kariabancroft
Copy link

TREK

What We're Looking For

Feature Feedback
Core Requirements
Git hygiene Good
Comprehension questions Yes
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, nice practice with grid and parallax
Under the Hood
Trip data is retrieved using from the API Yes
JavaScript is well-organized and easy to read Yes
HTML is semantic Yes
Overall Overall you did a nice job hitting the major learning goals of this assignment. Watch out for missing semi-colons which was the only JS issue I noticed in the code.

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