Conversation
…d of application layout
…look up active record querying
Media RankerWhat We're Looking For
Wonderful job overall Goeun! In general, this project meets most of the requirements and it looks great too. I'm also really glad to see it deployed. Unfortunately, I saw a few project requirements that I was looking for missing in this project, aka:
Similarly, a large theme of my comments on your code will be about hoping to refactor your code to be less hard-coded. This project's code relies a lot on the idea that there will always be the three categories Album, Book, and Movie. It'd be great to see code that is more extensible than that. Other than that, the code is really great, and I'm glad to see it pretty much all complete... even if you mentioned it was a submission of lateness and disorganization. Definitely meets the requirements-- good work overall |
| </h2> | ||
|
|
||
| <ul class="list-group top-ten-list"> | ||
| <% i = 0 %> |
There was a problem hiding this comment.
You are using this variable i to limit the loop to under ten. It definitely works! However, there may be better ways to do this: First of all, you could consider a 10.times loop with this... Or... My main concern is that there is a lil bit of logic in this view that I'm not comfortable with (aka... the looping part is fine, but the limiting to ten!) You might want to switch this logic into a model or controller ;) That would be my favorite solution!
| class User < ApplicationRecord | ||
| has_many :votes, dependent: :nullify | ||
|
|
||
| validates :username, presence: true, uniqueness: true, format: {with: /\A[-_a-z0-9]+\Z/}, length: {maximum: 15} |
There was a problem hiding this comment.
I like the validations! Definitely not part of the project requirements though. But hey-- you did it-- and tested it! Just want to call out that it was work that wasn't necessarily asked for.
| <%= yield %> | ||
|
|
||
| <footer> | ||
| don't steal me, k thnx |
| <li> underscores (_) </li> | ||
| </ul> | ||
| </p> | ||
| <%= form_with method: :post, url: sessions_path do |f| %> |
There was a problem hiding this comment.
Yay! A nice form_with with explicit method + url. Breaks the pattern of giving it a model, but definitely works/useful for us in this situation!
| <div class="form-group"> | ||
| <%= f.label :category %> | ||
| <div class="col-md-8"> | ||
| <%= f.select(:category, options_for_select([['Movie', 'movie'], ['Album', 'album'], ['Book', 'book']]))%> |
There was a problem hiding this comment.
This ends up being a good, working solution for now... But what if the categories became larger than 3 items? what if there were 10 categories? 100? It'd be nice if we found a way to make this array of arrays into a collection defined in the controller
| <%= render partial: 'layouts/form_errors', | ||
| locals: {model: @work} %> | ||
|
|
||
| <%= render partial: "form", locals: {directions: "Complete this form to edit media:", action_button: "Edit Media"} %> |
| </thead> | ||
| <tbody> | ||
| <% @works.sort.each do |work| %> | ||
| <% if work.category == "album" %> |
There was a problem hiding this comment.
These three tables end up being identical besides the category that you check on. There are a few ways to refactor this. One way is to use partials: You've used partials before with different things passed into it... (aka the form with different local messages for buttons) Could you do the same for this table? Let me know if you have questions!
| <% end %> | ||
| </tbody> | ||
| </table> | ||
| <br > |
There was a problem hiding this comment.
Try not to use a <br /> tag if possible -- brs are for line breaks, which is a display/layout thing!
| 🎬 | ||
| <% else @work.category == "book"%> | ||
| 📖 | ||
| <% end %> |
There was a problem hiding this comment.
This is view logic! I'm not mad about it, but just know: this can be solved with either controller logic... or CSS ;O
| # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html | ||
|
|
||
| root 'homes#index' | ||
| get '/homes', to: 'homes#index' |
There was a problem hiding this comment.
It might be nice to name this path, so you can reference it in the line above
| require "test_helper" | ||
| require 'pry' | ||
|
|
||
| describe Work do |
There was a problem hiding this comment.
Great job getting tests on relations and validations -- What about your custom methods?
| work = works(:nemo) | ||
|
|
||
| work.votes.length.must_equal 3 | ||
| work.votes[0].must_equal votes(:one) |
There was a problem hiding this comment.
do we want to check the order of what comes into the votes array? I'm not sure that it's important that we do. However, if you wanted to do this, I might instead check with
work.votes.include?( votes(:one) ).must_equal true
| it 'is invalid with a non-unique title' do | ||
| repeat = Work.new( | ||
| category: 'movie', | ||
| title: 'Finding Nemo', |
There was a problem hiding this comment.
it might be nice to re-inforce the point of this test by referencing the title of an existing work (defined in fixtures), aka
title: works(:nemo).title,
| end | ||
|
|
||
| it 'is invalid if publication date is greater than 2018' do | ||
| w = works(:heartburn) |
There was a problem hiding this comment.
I LOVE THIS VALIDATION! Not required, but still really goood!
2 participants
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
self.spotlightand `self.sorted'. They do nearly the same thing, which is to sort all Works by the length of the votes array (aka- how many votes each work has) except spotlight returns the first of that array .sessionandflash? What is the difference between them?@logged_in_useruntil user logs out or closes the browser.find_workin Work andfind_userin user finds the work and I don't have to repeat it in 5 different places.create. IDK if I'm allowed to do this? But if I am, the lesson would be that Controllers are there to be designated locations and guides but it's not technically confining