-
Notifications
You must be signed in to change notification settings - Fork 26
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
Anibel & Hayden - Edges Rideshare On Rails #20
base: master
Are you sure you want to change the base?
Conversation
…er show template to display trips
…hare-rails into add-status-column
…hare-rails into add-status-column
Rideshare RailsWhat We're Looking For
Good work with this Anibel and Hayden! The code looks good overall and the project looks great too I have a few comments for the code... My two "big" comments are: What happens if there are no trips associated with a driver? Then total would be 0.0 and trip_count is 0. Ruby evaluates 0.0 / 0 as NaN or "Not A Number." When it gets to the driver/show.html.erb view, NaN.to_s evaluates to just "NaN" so it shows up on the driver page as "NaN" on the page. It'll show up like this: "Minnie Dach The search feature for finding a specific driver or passenger is wonderful! Great work on that. Otherwise, in general, this project is great-- great work you two, despite being sick. Thanks for pushing through the challenges. |
if @passenger.save | ||
redirect_to passenger_path(@passenger) | ||
else | ||
render :new, status: :bad_request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonderful controller code! I love rendering a view and sending a status with it! That's the thoroughness and detail I like!
<li class= | ||
"tripsdriver"><%= link_to trip.driver.name, driver_path(trip.driver.id) %></li> | ||
<li class="tripspassenger"><%= link_to trip.passenger.name, passenger_path(trip.passenger.id) %></li> | ||
<li class="tripsprice">$<%=trip.cost%></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment, but this view doesn't format the cost to have a decimal two places in ;) trips costs are in the thousands haha
<li class="nav-links"> | ||
<%= link_to "Drivers"%> | ||
<div class="dropdown-content"> | ||
<p class="unlast-item"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just out of curiosity, what does this class represent or do? I didn't find other uses of this class name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was originally used to put a decorative line between menu options but was later scrapped. class should have been removed :)
|
||
resources :trips, except: [:index, :edit] | ||
|
||
get "/pages/:page" => "pages#show" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, especially in conjunction with the implementation of the show action for PagesController... Though I'm not convinced that it needed to be like this-- why not just have a specific route for a "home" action defined in the PagesController, instead of using a URL param?
aka
get "/home" => "pages#home"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was set up so that many static pages could be created by any name. that obv wasn't used here though!
Rideshare-Rails
Congratulations! You're submitting your assignment! These comprehension questions should be answered by both partners together, not by a single teammate.
Comprehension Questions