Skip to content
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

Wave 2 #103

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open

Wave 2 #103

wants to merge 42 commits into from

Conversation

jm-rives
Copy link

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall a good amount of work.

You'll need to work on ordering the media (see my notes below), fixing some links and using Foundation for layout, but you're pretty close!

</tr>
<% @movies[0..9].each do |movie| %>
<tr>
<td><%= link_to movie.name %></td>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably also want to include the path to the movie's show page in the link. Right now the link doesn't go anywhere.

@movies = Movie.all
@books = Book.all
@albums = Album.all

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a good idea to simply get the top 10 movies, books & albums.

You can do that with: Movie.order(rank: :desc).limit(10)

So it orders the movies by rank and then limits the response to 10 items.

@@ -0,0 +1,63 @@
class AlbumsController < ApplicationController

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future you could consider using a filter with a before action.

before_action :find_album, only: [:show, :edit, :upvote, :destroy]

...

private
def find_album
@MyAlbum = Album.find_by(params[:id].to_i)
end

@@ -0,0 +1,49 @@
<%= image_tag "owl_media_ranker.jpg" %>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this page responsive, you'll need to use a section for each type of media, with using the Foundation grid layout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants