-
Notifications
You must be signed in to change notification settings - Fork 48
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
Carly Reiter - Edges - Media Ranker #25
base: master
Are you sure you want to change the base?
Conversation
…w files (with Dan's help).
… works index page.
Media RankerWhat We're Looking For
Hi Carly! You hit the following learning goals really well:
I'm sure you know, but you didn't get to the following:
This was a tough project! I definitely think that a lot of the submitted code is good. I have a feeling that some of this project was a time crunch, and some of it was working through understanding how to flex Rails to your will in a more custom manner. Maybe the situation has changed now that we've gone a few weeks. Let's have a talk about this sometime soon-- feel free to bring your questions! I'm going to make a few comments on some things that I think could be tidied up, or some ways to look at this code in a new perspective. I understand that this feedback may be very late-- sorry about that |
class VotesController < ApplicationController | ||
|
||
def index | ||
@votes = Vote.all |
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.
When do we go down this path?
@vote = Vote.find_by(id: params[:id]) | ||
if @vote.nil? | ||
head :not_found | ||
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.
Hm, which requirement/user story were you working on when trying to view a specific vote?
@@ -0,0 +1 @@ | |||
<h1>Edit users page</h1> |
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.
is this view, or the users/new
view, ever used?
<section> | ||
<%= link_to "Back to media ranks", works_path, class: "btn btn-primary" %> | ||
<%= link_to "Edit", edit_work_path, class: "btn btn-primary" %> | ||
<%=link_to "Upvote", upvote_path, method: :post, class: "btn btn-primary" %> |
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 an fyi, the syntax I used to comment this code out is:
<%#link_to "Upvote", upvote_path, method: :post, class: "btn btn-primary" %>
resources :votes, only: [:index, :create, :update] | ||
member do | ||
post 'upvote' | ||
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.
interesting, but not quite what we want. You're correct that you want to nest something about post
requests and upvoting within a work, because then we get to make a request that is inherently related to a work (and it'll definitely have a work ID associated with it when it's nested!)... You have something closer to what we expected below in this file, where you nested resources :votes
into resources :users
. In general, we know that vote are tied to both a user and a work, so therefore at the time of creating a vote (aka the "upvote"), we need to have a way to reference a work and reference a user. Using routes as a mechanism for having a way to reference a work is a good strategy.
|
||
resources :works do | ||
|
||
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.
When you have resources :works
above this block already declared, you don't need to re-declare it empty down here. The above block accomplishes the whole "making routes for `works" bit!
|
||
resources :users do | ||
resources :votes, only: [:index, :create] | ||
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.
Overall in this file, you're playing around and experimenting with how to make the votes paths. Ultimately, I'd say: we don't want any routes paths that are on their own (not-nested), and we don't need the "create" path for a vote nested under a user. (There's also no requirement that would be better off if we nested the votes index route under a user, too, but I could see someone argue for it.)
|
||
resources :sessions, only: [:new, :create] | ||
|
||
post '/sessions/logout', to: 'sessions#logout', as: 'logout' |
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.
Nice! You had a specific action and URL that you wanted to make as a custom route, you knew which controller and action it went to, and the name you would refer to it within the app code.
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
session
andflash
? What is the difference between them?