From 3675e16667448dabecf67dd81522c2412f770292 Mon Sep 17 00:00:00 2001 From: RobertAKARobin Date: Thu, 13 Aug 2015 20:44:59 -0400 Subject: [PATCH] Robin's comments --- Gemfile.lock | 3 --- app/assets/javascripts/application.js | 11 +++++++++++ app/assets/stylesheets/application.css | 7 +++++++ app/controllers/articles_controller.rb | 3 ++- app/controllers/users_controller.rb | 3 ++- app/helpers/application_helper.rb | 1 + app/models/guardian_article.rb | 2 +- app/views/articles/_nav.html.erb | 2 ++ config/routes.rb | 1 + 9 files changed, 27 insertions(+), 6 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 95e0b02..3aab2da 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -177,6 +177,3 @@ DEPENDENCIES turbolinks uglifier (>= 1.3.0) web-console (~> 2.0) - -BUNDLED WITH - 1.10.5 diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index bfff697..58730e0 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -20,6 +20,12 @@ $(document).ready(function(){ var counter = 1; // this function controls the clicks on the article navigation bar to sort content by time + /* + // DRYing: + function animate_width(el, amount, display_val){ + el.animate({width: amount}, 800).css("display", display_val) + } + */ $(".columns").click(function(){ var self = this $('h1').each(function(){ @@ -28,6 +34,7 @@ $(document).ready(function(){ $(this).parent().animate({ width: "100%" }, 800, function() {}) + // This last argument, the function, is an optional parameter $(this).parent().css("display", "block") } else { $(this).parent().animate({ @@ -47,6 +54,8 @@ $(document).ready(function(){ // this function controls clicks on Guardian content (if it has been loaded after a search) and controls expanding/descreasing the size of the columns $(".guardianClick").click(function(){ + // No need to declare twice + // var self = this if (counter % 2 != 0) { var self = this; @@ -73,3 +82,5 @@ $(document).ready(function(){ }); // closes guardianClick function }); // closes document.ready + +// If you have to comment your code to remind yourself which brackets close what, you should probably extract a bunch of your code into new, smaller methods. diff --git a/app/assets/stylesheets/application.css b/app/assets/stylesheets/application.css index 8392169..fc02ff0 100644 --- a/app/assets/stylesheets/application.css +++ b/app/assets/stylesheets/application.css @@ -187,6 +187,13 @@ border-bottom: 5px solid #FFAF94; } + /* + Might make a class for all of these above with: + border-bottom-style: solid; + border-bottom-width: 5px; + That way, you only need to specify the color on each of them. + */ + /*************************content container elements**************************/ .no-overflow { height: 30px; diff --git a/app/controllers/articles_controller.rb b/app/controllers/articles_controller.rb index 571b607..c249d8e 100644 --- a/app/controllers/articles_controller.rb +++ b/app/controllers/articles_controller.rb @@ -1,13 +1,14 @@ class ArticlesController < ApplicationController ######## CRUD ROUTES ######## - +# Don't call them names! They're very sensitive. #index def index if params[:query] != nil @guardian_article = GuardianArticle.new(params[:query]) end @articles = Article.all + # Could save some text by making a custom current_user method in your ApplicationController @user = User.find(session[:user]["id"]) end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index b41615a..7b6511d 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -56,6 +56,7 @@ def sign_up! def sign_in end +# This is a really long method! Granted, it's pretty much just taken from what we did in class. Still, I'd suggest seeing if you can keep all your methods to 5 lines or fewer. This is an actual programming methodology used by many. def sign_in! @user = User.find_by(username: params[:username]) if !@user @@ -68,7 +69,7 @@ def sign_in! message = "You're signed in, #{@user.username}!" cookies[:username] = { value: @user.username, - +# Watch out for the dangling comma here } session[:user] = @user redirect_to articles_path diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index a21937a..d17e73e 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -13,6 +13,7 @@ def most_read?(article) end # manages length class assigned to Guardian articles + # This method is included in the Article model. How could you DRY it up? def ext_length_class(result) article_length = sanitize(result['fields']['body'], :tags=>[]).split(" ").length if article_length <= 400 diff --git a/app/models/guardian_article.rb b/app/models/guardian_article.rb index cd86318..80a9935 100644 --- a/app/models/guardian_article.rb +++ b/app/models/guardian_article.rb @@ -1,5 +1,5 @@ require 'httparty' - +# Great use of a custom model class GuardianArticle API_URL = 'http://content.guardianapis.com' attr_reader :response, :results, :length diff --git a/app/views/articles/_nav.html.erb b/app/views/articles/_nav.html.erb index 4184ba1..2b07b2a 100644 --- a/app/views/articles/_nav.html.erb +++ b/app/views/articles/_nav.html.erb @@ -5,3 +5,5 @@
6 Minutes
10 Minutes
+ + diff --git a/config/routes.rb b/config/routes.rb index 65615ab..c0c8882 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -7,6 +7,7 @@ # get '/pins', to: 'pins#index' # post '/pins', to: 'pins#create' + # Might call this a "save" instead of a like get '/likes', to: 'likes#create' # get '/likes/:id', to: 'likes#destroy' # post '/likes', to: 'likes#create'