-
Notifications
You must be signed in to change notification settings - Fork 42
Ampers - Lily #44
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
base: master
Are you sure you want to change the base?
Ampers - Lily #44
Conversation
…needs to be tested
…ks, but doesn't display username
… maybe a few other edits?
…ges across program
…in css; time to look at media queries
…eturn top 10 of each category, old return all
Media RankerWhat We're Looking For
Great job! The site functions as expected and I think you hit all of the goals for this project. The code generally looks good. There's a few unit tests that I would have liked to see and one place of refactoring that I'm commenting on. Also, your main page doesn't limit the listed "top ten" works to ten? Otherwise I think it's good work! |
| <div class="top-bar-right"> | ||
| <ul class="menu"> | ||
| <% if session[:user_id] != nil %> | ||
| <li><%= link_to "Logged in as #{User.get_username(session[:user_id])}" , user_path(session[:user_id]), method: :get, class: "button"%></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.
You reference session[:user_id] a lot in this section of code:
- you use a conditional on it
- you get the username here through
{User.get_username(session[:user_id])} - you use it for the user_path param
You already declare before_action :find_user in each controller, so each view will have a value on @user... maybe you could use that value?
|
|
||
| validates :publication_year, presence: true, numericality: {only_integer: true, greater_than: 0, less_than_or_equal_to: Date.current.year} | ||
|
|
||
| KINDS = ["Album", "Book", "Movie"] |
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.
I like that you name the categories in this array as a constant!
You could push this further to make it an array of symbols
| @@ -0,0 +1,18 @@ | |||
| <%= form_for @work do |f| %> | |||
| <%= f.label :category %> | |||
| <%= f.select :category, ["Album", "Book", "Movie"] %> | |||
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 hardcoded the list of categories here, but it might've been cool to find a rails form helper to populate a select with all unique categories that works have. ;) Just a thought.
| <%= f.text_field :description %> | ||
|
|
||
| <%= f.submit class: "button"%> | ||
| <% 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.
watch your indentation :)
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?