Conversation
Database setup
add movies/reviews routes
Implemented restful routes for Movie controller
Added a basic showpage, moving cause of sun
Basic html views for movies
reviews mostly complete
Created basic user show page
Backlogs fav buttons
top box office and review fix
style movie show some
buttons prettied
Activity feed
Rspec happyface
philsof
left a comment
philsof
left a comment
There was a problem hiding this comment.
@IuliiaKot @mabenson00 @paulizleet @jordanyryan See inline notes for tips and comments
| @@ -0,0 +1,25 @@ | |||
| class BacklogsController < ApplicationController | |||
There was a problem hiding this comment.
What is a backlog? Is this your watchlist? Please use names that describe what is being represented.
| class CreateReviews < ActiveRecord::Migration[5.0] | ||
| def change | ||
| create_table :reviews do |t| | ||
| t.string :movie_id |
There was a problem hiding this comment.
Best practice is to index all foreign key fields. t.references does this automatically, but here you would need to be explicit:
t.string :movie_id, index: trueNote: this goes for all movie_id fields in all of your tables
| def change | ||
| create_table :comments do |t| | ||
| t.string :movie_id | ||
| t.references :user, foreign_key: true |
There was a problem hiding this comment.
How could you have implemented comments on other comments? A la Hacker News conversations?
| def activity_feed | ||
| feed = [] | ||
| feed << self.favorites | ||
| feed << self.backlogs |
There was a problem hiding this comment.
How could you have also tracked activity that removes records from the database, e.g. when a movie is unfavorited and/or removed from a watchlist?
| feed << self.backlogs | ||
| feed << self.comments | ||
| feed << self.reviews | ||
| feed.flatten |
There was a problem hiding this comment.
FYI Flattening an array is expensive. What if you had a log table that recorded every user activity? That would come in handy when querying (see next note).
| module ReviewsHelper | ||
|
|
||
| def reviews_find_movie | ||
|
|
|
|
||
| <%= javascript_include_tag 'application' %> | ||
|
|
||
| <link rel="stylesheet" href="https://cdnjs.cloudflare.com/ajax/libs/materialize/0.98.1/css/materialize.min.css"> |
There was a problem hiding this comment.
Let's utilize the asset pipeline to bring in all of the CSS and JS assets. More info on why this is a best practice, and how to do so, here
| </div> | ||
| </footer> | ||
| <script type="text/javascript" charset="utf-8"> | ||
| $(window).load(function() { |
There was a problem hiding this comment.
Let's move this into the JS assets
|
|
||
|
|
||
| <%= form_for :movies url: movie_path, method: :patch do |f| %> | ||
| <% if @article.errors.any? %> |
| <%=link_to movie[:Title], "/movies/#{movie[:imdbID]}", class: "homepage-link" %> | ||
| </div> | ||
| <div class="overlay-stars"> | ||
| <%= Review.for_movie(movie[:imdbID])[-1].stars %> |
There was a problem hiding this comment.
Let's not reach into an array in the view. Have the model deliver to the view exactly what it needs.
4 participants
DO NOT MERGE THIS PULL REQUEST!