-
Notifications
You must be signed in to change notification settings - Fork 47
Sockets - Grace #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
base: master
Are you sure you want to change the base?
Sockets - Grace #26
Conversation
… views and form added
Media RankerWhat We're Looking For
This is a strong submission. Your implementation matches the demo site very closely, and I would say the learning goals for this assignment were definitely met. However, there are a number of bugs in this app that were small and straightforward to fix, but which had a devastating effect on the user experience, like not showing any works on the splash or index pages, not displaying the login form, or not being able to upvote or delete from the show page. What that tells me is that you're relying exclusively on your tests to tell you whether your site works. Your testing on this assignment is really strong, but there were a couple of places where it didn't quite match up with what the user is doing. These are all things that manually running through the workflows of your site would have caught immediately. The takeaway is, before you submit your next assignment, spend 10 minutes running through it manually and making sure it works! |
| <% form_with model: @user, url: login_path do |f| %> | ||
| <div class="form-group"> | ||
| <%= f.label :name %> | ||
| <%= f.text_field :name, placeholder: "Username", class:"form-control" %> |
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.
Line 3 should be a value tag (<%=), otherwise the form won't appear on the page.
| def self.get_media_categories | ||
| books = self.get_media("books") | ||
| albums = self.get_media("albums") | ||
| movies = self.get_media("movies") |
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.
Here you search for works with category albums, books and movies, but your database seeds and the dropdown menu on your new work form use the singular version of these (album, book, movie). As a result, your index and splash pages don't display any works.
| <%= link_to "Edit", edit_work_path(@work.id), class: "btn btn-primary btn-lg" %> | ||
| <%= link_to "Upvote", class: "btn btn-primary btn-lg" %> | ||
| <%= link_to "Delete", class: "btn btn-danger btn-lg" %> | ||
| </section> |
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.
The links on line 13 and 14 are missing URLs, so neither of them works.
| <section class="spotlight"> | ||
| <% media = Work.all.sample%> | ||
| <h2 class="spotlight__header"> | ||
| <span class="spotlight__header--prefix"> Media Spotlight </span> |
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 should probably use the @spotlight variable that you set up in the controller. As-is it shows a random work, not the one with the most votes.
| <section class="top-ten__list-container"> | ||
| <h2>Top Movies</h2> | ||
| <%= render "top_ten", top: @top_ten[2] %> | ||
| </section> |
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 love the way you've used a partial to DRY this up - it makes this code much more readable.
| get "/", to: "homepages#index" | ||
| resources :works, only: [:index, :show, :new, :create, :edit, :update, :destroy] | ||
| resources :works do | ||
| resources :votes, only: [:create] |
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 code is a little redundant:
- On line 6, you limit
resourcesto generate "only" all 7 of the RESTful routes - On line 7, you re-generate all 7 routes again
The change here would be to omit line 6.
| post "/login", to: "users#login" | ||
| get "/users/current", to: "users#current", as: "current_user" | ||
|
|
||
| resources :users, only: [:index, :show] |
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.
Good work generating only the routes your application needs.
| class Vote < ApplicationRecord | ||
| belongs_to :user | ||
| belongs_to :work | ||
| validates :user, uniqueness: { scope: [:work], message: "has already voted for this work" } |
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.
Good work getting this tricky uniqueness scope figured out!
| def self.top_media | ||
| top_works = get_media_categories.map do |category| | ||
| category[0..9] | ||
| 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.
I love the way the methods in this file build on each other, each providing a small bit of functionality. This is an excellent example of functional decomposition.
|
|
||
| it "will create an account for and login a new user" do | ||
| @login_data[:user][:name] = "new user" | ||
|
|
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.
Good work catching the two different success cases here.
Is there a failure case? What if the user forgets to type in their name before logging in?
Media Ranker
Congratulations! You're submitting your assignment!
Comprehension Questions
sessionandflash? What is the difference between them?