Feedback PR only#25
Conversation
Lucy station crud
30 cities crud
Make forms consistent
56 consistent ids
Will paginate
65 update delete formatting
Pass station_spec
Add indexes on foreign keys
Tinker with CSS
Updates to trips
There was a problem hiding this comment.
@ski-climb @lucyconklin @jdconrad89 @akintner great job on this 🎉 let me know if you have questions on any of my comments/suggestions!
also - great work on committing often 👍
|
|
||
| class BikeShareApp < Sinatra::Base | ||
| set :method_override, true | ||
| include WillPaginate::Sinatra::Helpers |
| end | ||
|
|
||
| get '/stations/' do | ||
| @stations = Station.paginate(:page => params[:page], :per_page => 30) |
| post '/stations' do | ||
| city = City.find_or_create_by(name: params[:city]) | ||
| station = Station.create(params[:station]) | ||
| station.city_id = city.id |
There was a problem hiding this comment.
rather than setting the city_id manually, we could do something like this to leverage the full power of ActiveRecord:
city = City.find_or_create_by(name: params[:city])
city.stations.create(params[:station])then, we don't need the save 👍
| end | ||
|
|
||
| get '/cities/new' do | ||
| erb :"cities/new" |
There was a problem hiding this comment.
love that your nesting your view templates within folders 👍
| validates :precipitation_inches, presence: true | ||
| validates :measurement_date, presence: true | ||
|
|
||
| def present_date |
| "Subscribers = #{hash[subscriber_id]}" | ||
| end | ||
|
|
||
| def self.user_percentage |
There was a problem hiding this comment.
this method's doing a lot of stuff - can we break it down?
| name: subscription_name | ||
| ) | ||
|
|
||
| zipcode = Zipcode.find_or_create_by( |
| <li>November: <%= @trips.rides_per_month(11) %></li> | ||
| <li>December: <%= @trips.rides_per_month(12) %></li> | ||
| </ul> | ||
| <p><%= @trips.rides_per_year(2016) %><br?></p> |
There was a problem hiding this comment.
this snippet here is a great candidate for a partial - since the html here and code is pretty similar (except for the year). we can re-use this piece of html by putting it in a partial 👍 if you want to dive into that further and have questions, let me know!
| <td><%= trip.id %></td> | ||
| <td><%= trip.duration_in_seconds%></td> | ||
| <td><%= trip.start_date%></td> | ||
| <td><%= @stations.find_by(id: trip.start_station_id).name%></td> |
There was a problem hiding this comment.
this is an example of querying your database in your views since you're calling find_by - how can we remove this logic and keep our views simply displaying data that the controller fetches for us?
| describe "When a user wants to delete the weather conditions for a day" do | ||
|
|
||
| context "they can delete from the show" do | ||
| let(:today) { Date.today } |
There was a problem hiding this comment.
love that you're using let 👍
4 participants
do not merge