Adapt routes and views #21

Polo2 · 2018-09-27T15:43:35Z

Basic set up to navigate between dependencies and comments

michaelbaudino · 2018-09-28T09:02:54Z

app/controllers/comments_controller.rb

@@ -6,7 +6,7 @@ class CommentsController < ApplicationController
  # GET /comments
  # GET /comments.json
  def index
-    @comments = Comment.all
+    @comments = Comment.all.sort_by { |c| c.dependency.name }


We should actually sort this by created_at, imho 🤔

And whatever we decide for the sorting criteria, it belongs to the model, not
the controller.
Also, wouldn't #sort_by do the sort in Ruby? If so, we should let PostgreSQL
do the work for us IMO (Comment.order name: :asc, Comment.order created_at: :asc…)

michaelbaudino · 2018-09-28T09:03:09Z

app/views/comments/_form.html.erb

@@ -23,7 +23,7 @@

  <div class="field">
    <%= form.label :dependency_id %>
-    <%= form.text_field :dependency_id %>
+    <%= form.select :dependency_id, Dependency.all.collect {|d| [ d.name, d.id ] } %>


I believe there's a Rails helper for that.

Line is too long;

I'd favor #map over #collect (but as said there might be an helper to
remove/simplify this part);

Does our coding style allow this syntax for the block? Shouldn't this be:
{ |d| [d.name, d.id] }

michaelbaudino · 2018-09-28T09:08:45Z

db/seeds.rb

+puts "#{Dependency.count} created"
+puts "#{Comment.count} created"
+
+puts "Happy Growing !"


This file shouldn't be here.

ghost · 2018-09-28T09:57:45Z

app/controllers/comments_controller.rb

@@ -6,7 +6,7 @@ class CommentsController < ApplicationController
  # GET /comments
  # GET /comments.json
  def index
-    @comments = Comment.all
+    @comments = Comment.all.sort_by { |c| c.dependency.name }


And whatever we decide for the sorting criteria, it belongs to the model, not
the controller.
Also, wouldn't #sort_by do the sort in Ruby? If so, we should let PostgreSQL
do the work for us IMO (Comment.order name: :asc, Comment.order created_at: :asc…)

ghost · 2018-09-28T10:01:27Z

app/views/comments/_form.html.erb

@@ -23,7 +23,7 @@

  <div class="field">
    <%= form.label :dependency_id %>
-    <%= form.text_field :dependency_id %>
+    <%= form.select :dependency_id, Dependency.all.collect {|d| [ d.name, d.id ] } %>


Line is too long;

I'd favor #map over #collect (but as said there might be an helper to
remove/simplify this part);

Does our coding style allow this syntax for the block? Shouldn't this be:
{ |d| [d.name, d.id] }

ghost · 2018-09-28T10:04:58Z

app/views/dependencies/index.html.erb

@@ -14,6 +14,7 @@
    <% @dependencies.each do |dependency| %>
      <tr>
        <td><%= dependency.name %></td>
+        <td><%= "#{dependency.comments.count} comments" %></td>


<%= "#{dependency.comments.count} comments" %>

should be:

<%= dependency.comments.count %> comments

ghost · 2018-09-28T10:05:27Z

app/views/dependencies/index.html.erb

@@ -25,3 +26,4 @@
 <br>

 <%= link_to 'New Dependency', new_dependency_path %>
+<%= link_to "See Comments", comments_path %>


A final \n is missing at end of file.

thibaultdalban · 2018-09-28T13:07:30Z

app/views/dependencies/index.html.erb

@@ -14,6 +14,7 @@
    <% @dependencies.each do |dependency| %>
      <tr>
        <td><%= dependency.name %></td>
+        <td><%= dependency.comments.count %> comments</td>


Use I18n or remove this line

thibaultdalban · 2018-09-29T14:48:36Z

app/views/comments/_form.html.erb

@@ -23,7 +23,8 @@

  <div class="field">
    <%= form.label :dependency_id %>
-    <%= form.text_field :dependency_id %>
+    <%= select_tag(:dependency_id,


Please use form.select instead of select_tag, specially in rails 5.2 with the new form_with method.

Polo2 requested review from a user, michaelbaudino and thibaultdalban September 27, 2018 15:43

michaelbaudino temporarily deployed to toolbox-staging-pr-21 September 27, 2018 15:43 Inactive

michaelbaudino suggested changes Sep 28, 2018

View reviewed changes

ghost suggested changes Sep 28, 2018

View reviewed changes

Polo2 force-pushed the adapt-routes-and-views branch from 9db287f to d56f90b Compare September 28, 2018 12:36

Polo2 temporarily deployed to toolbox-staging-pr-21 September 28, 2018 12:36 Inactive

Polo2 force-pushed the adapt-routes-and-views branch from d56f90b to c0d7f95 Compare September 28, 2018 12:38

Polo2 temporarily deployed to toolbox-staging-pr-21 September 28, 2018 12:38 Inactive

Polo2 requested review from michaelbaudino and a user September 28, 2018 12:39

thibaultdalban suggested changes Sep 28, 2018

View reviewed changes

Polo2 force-pushed the adapt-routes-and-views branch from c0d7f95 to 3f30003 Compare September 28, 2018 13:30

Polo2 deployed to toolbox-staging-pr-21 September 28, 2018 13:30 View deployment

Polo2 requested a review from thibaultdalban September 28, 2018 13:38

thibaultdalban self-assigned this Sep 28, 2018

thibaultdalban approved these changes Sep 28, 2018

View reviewed changes

thibaultdalban assigned Polo2 and unassigned thibaultdalban Sep 28, 2018

thibaultdalban suggested changes Sep 29, 2018

View reviewed changes

Adapt views.haml for dependencies and comments

c3721ad

Polo2 force-pushed the adapt-routes-and-views branch from 3f30003 to c3721ad Compare October 4, 2018 13:34

Polo2 had a problem deploying to toolbox-staging-pr-21 October 4, 2018 13:34 Failure

Polo2 requested a review from thibaultdalban October 4, 2018 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adapt routes and views #21

Adapt routes and views #21

Polo2 commented Sep 27, 2018

michaelbaudino Sep 28, 2018

ghost Sep 28, 2018

michaelbaudino Sep 28, 2018

ghost Sep 28, 2018

michaelbaudino Sep 28, 2018

ghost Sep 28, 2018

ghost Sep 28, 2018

ghost Sep 28, 2018

ghost Sep 28, 2018

thibaultdalban Sep 28, 2018

thibaultdalban Sep 29, 2018

Adapt routes and views #21

Are you sure you want to change the base?

Adapt routes and views #21

Conversation

Polo2 commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment