-
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
Katricia/Karis - Edges - RideShareRails #18
base: master
Are you sure you want to change the base?
Conversation
Merge trip view into master
…ll trips for single passenger
…eholder lists to driver list
… doesnt show the driver or new passenger id yet)
Merge katriciabranch and karis branch
Karis branch to master
…otal_earnings methods for driver
Katricia branch2
… for both passengers n trips
Add form partial for error handling
Correct driver total_earnings method
Rideshare RailsWhat We're Looking For
|
|
||
nav li :visited { | ||
color: #d08b2536; | ||
} |
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.
This doesn't style unvisited links (if the user is visiting your site for the first time), which are really hard to read.
|
||
def average_rating | ||
return trips.sum(&:rating).to_f/ trips.length | ||
end |
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.
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.
<section class="passenger-trips"> | ||
<h3> Passenger Trips </h3> | ||
<table id="driver-trips-table"> | ||
<tr> |
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.
You have almost exactly the same code to render the table of trips on the passenger show
page and the driver show
page. Could you DRY this up with a partial somehow?
|
||
driver = (Driver.all.select { |d| d.availability == true }).sample | ||
|
||
@trip = passenger.trips.new(driver_id: driver.id, passenger_id: passenger.id, date: Date.today, rating: 0, cost: 0) |
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.
All of the work done in this method could be moved to a model method. An instance method on Passenger
would probably work well, something like Passenger#request_trip
.
def update | ||
@driver = Driver.find(params[:id]) | ||
|
||
if @driver.update(driver_params) |
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's interesting to me that in create
you store the strong params in an intermediate local variable, but in update
you pass it directly into the model. Using the local variable is fine if it makes the code clearer to you, but you should follow the same pattern in both places.
resources :drivers | ||
|
||
resources :trips | ||
|
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.
Do you need all 7 restful routes for :trips
? I think that both :new
and un-nested :create
are not used.
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