From 4627372a6277e4da4cbcef7f4f55b8b4ac1ea4e1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 10 May 2020 20:14:15 +0200
Subject: [PATCH] Use a <ul> tag for a list of comments

We were using a <ul> tag for a single comment, where the first element
of the list was the comment itself and the second element was the list
of replies.

IMHO it makes more sense to have a list of all comments, where every
element is a comment and inside it there's a list of replies.

We're also rendering the list even if it has no children so it's easier
to add comments through JavaScript. Then we use the :empty CSS selector
to hide the list if it's empty. However, since ERB adds whitespace if we
structure our code the usual way and current browsers don't recognize
elements with whitespace as empty, we have to use the `tag` helper so no
whitespace is added.
---
 app/assets/javascripts/comments.js            |  9 +++-----
 app/assets/stylesheets/layout.scss            | 20 +++++++++++-----
 app/views/comments/_comment.html.erb          | 23 ++++++-------------
 app/views/comments/_comment_list.html.erb     |  7 ++++++
 app/views/comments/_comment_tree.html.erb     |  4 +---
 app/views/comments/create.js.erb              |  2 +-
 app/views/debates/_comments.html.erb          |  4 +---
 .../legislation/proposals/_comments.html.erb  |  4 +---
 app/views/polls/_comments.html.erb            |  4 +---
 app/views/proposals/_comments.html.erb        |  4 +---
 app/views/topics/_comments.html.erb           |  5 +---
 .../comments/budget_investments_spec.rb       | 18 +++++++--------
 spec/system/comments/debates_spec.rb          | 18 +++++++--------
 .../comments/legislation_annotations_spec.rb  | 18 +++++++--------
 .../comments/legislation_questions_spec.rb    | 18 +++++++--------
 spec/system/comments/proposals_spec.rb        | 18 +++++++--------
 16 files changed, 83 insertions(+), 93 deletions(-)
 create mode 100644 app/views/comments/_comment_list.html.erb

diff --git a/app/assets/javascripts/comments.js b/app/assets/javascripts/comments.js
index 250efc340..f2c5411ca 100644
--- a/app/assets/javascripts/comments.js
+++ b/app/assets/javascripts/comments.js
@@ -2,14 +2,11 @@
   "use strict";
   App.Comments = {
     add_comment: function(parent_id, response_html) {
-      $(response_html).insertAfter($("#js-comment-form-" + parent_id));
+      $(".comment-list:first").prepend($(response_html));
       this.update_comments_count();
     },
     add_reply: function(parent_id, response_html) {
-      if ($("#" + parent_id + " .comment-children").length === 0) {
-        $("#" + parent_id).append("<li><ul id='" + parent_id + "_children' class='no-bullet comment-children'></ul></li>");
-      }
-      $("#" + parent_id + " .comment-children:first").prepend($(response_html));
+      $("#" + parent_id + " .comment-list:first").prepend($(response_html));
       this.update_comments_count();
     },
     update_comments_count: function() {
@@ -46,7 +43,7 @@
       });
 
       $("body").on("click", ".js-toggle-children", function() {
-        $("#" + $(this).data().id + "_children").toggle("slow");
+        $(this).closest(".comment").find(".comment-list:first").toggle("slow");
         $(this).children(".far").toggleClass("fa-minus-square fa-plus-square");
         $(this).children(".js-child-toggle").toggle();
         return false;
diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss
index 82aee720f..977c6fe6b 100644
--- a/app/assets/stylesheets/layout.scss
+++ b/app/assets/stylesheets/layout.scss
@@ -2084,6 +2084,7 @@ table {
   }
 
   .comment {
+    line-height: $list-lineheight;
     margin: $line-height / 4 0;
     position: relative;
 
@@ -2171,12 +2172,19 @@ table {
   }
 }
 
-.comment-children {
-  border-left: 1px dashed $border;
-  display: inline-block;
-  margin-left: rem-calc(16);
-  padding-left: rem-calc(8);
-  width: 100%;
+.comment-list {
+  margin: $line-height / 4 0;
+
+  .comment-list {
+    border-left: 1px dashed $border;
+    display: inline-block;
+    padding-left: rem-calc(8);
+    width: 100%;
+  }
+
+  &:empty {
+    display: none;
+  }
 }
 
 .comment-info {
diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb
index fec693a48..443c69a01 100644
--- a/app/views/comments/_comment.html.erb
+++ b/app/views/comments/_comment.html.erb
@@ -1,7 +1,7 @@
 <% valuation = local_assigns.fetch(:valuation, false) %>
 <% cache [locale_and_user_status(comment), comment, commentable_cache_key(comment.commentable), comment.author] do %>
-  <ul id="<%= dom_id(comment) %>" class="comment no-bullet small-12">
-    <li class="comment-body">
+  <div id="<%= dom_id(comment) %>" class="comment small-12">
+    <div class="comment-body">
       <% if comment.hidden? || comment.user.hidden? %>
         <% if comment.children.size > 0 %>
           <div class="callout secondary">
@@ -86,7 +86,7 @@
           <% end %>
 
           <% if comment.children.size > 0 %>
-            <%= link_to "", class: "js-toggle-children relative", data: { "id": "#{dom_id(comment)}" } do %>
+            <%= link_to "", class: "js-toggle-children relative" do %>
               <span class="far fa-minus-square"></span>
               <span class="js-child-toggle" style="display: none;"><%= t("comments.comment.responses_show", count: comment.children.size) %></span>
               <span class="js-child-toggle"><%= t("comments.comment.responses_collapse", count: comment.children.size) %></span>
@@ -113,17 +113,8 @@
           <% end %>
         </div>
       <% end %>
-    </li>
-    <% unless child_comments_of(comment).empty? %>
-    <li>
-      <ul id="<%= dom_id(comment) %>_children" class="no-bullet comment-children">
-        <% child_comments_of(comment).each do |child| %>
-          <li>
-            <%= render "comments/comment", { comment: child, valuation: valuation } %>
-          </li>
-        <% end %>
-      </ul>
-    </li>
-    <% end %>
-  </ul>
+    </div>
+
+    <%= render "comments/comment_list", comments: child_comments_of(comment), valuation: valuation %>
+  </div>
 <% end %>
diff --git a/app/views/comments/_comment_list.html.erb b/app/views/comments/_comment_list.html.erb
new file mode 100644
index 000000000..26138c7f5
--- /dev/null
+++ b/app/views/comments/_comment_list.html.erb
@@ -0,0 +1,7 @@
+<% valuation = local_assigns.fetch(:valuation, false) %>
+
+<%= tag.ul class: "no-bullet comment-list" do %>
+  <% comments.each do |comment| %>
+    <%= tag.li render("comments/comment", { comment: comment, valuation: valuation }) %>
+  <% end %>
+<% end %>
diff --git a/app/views/comments/_comment_tree.html.erb b/app/views/comments/_comment_tree.html.erb
index 88d3d0f0b..055f94e4d 100644
--- a/app/views/comments/_comment_tree.html.erb
+++ b/app/views/comments/_comment_tree.html.erb
@@ -35,9 +35,7 @@
           <%= render "shared/login_to_comment" %>
         <% end %>
 
-        <% comment_tree.root_comments.each do |comment| %>
-          <%= render "comments/comment", { comment: comment, valuation: valuation } %>
-        <% end %>
+        <%= render "comments/comment_list", comments: comment_tree.root_comments, valuation: valuation %>
         <%= paginate comment_tree.root_comments %>
       </div>
     </div>
diff --git a/app/views/comments/create.js.erb b/app/views/comments/create.js.erb
index aa2bd9b1c..ff8c068bf 100644
--- a/app/views/comments/create.js.erb
+++ b/app/views/comments/create.js.erb
@@ -1,4 +1,4 @@
-var comment_html = "<%= j(render @comment) %>"
+var comment_html = "<li><%= j(render @comment) %></li>"
 
 <% if @comment.root? -%>
   var commentable_id = "<%= dom_id(@commentable) %>";
diff --git a/app/views/debates/_comments.html.erb b/app/views/debates/_comments.html.erb
index cf7f553d0..46cf3fb57 100644
--- a/app/views/debates/_comments.html.erb
+++ b/app/views/debates/_comments.html.erb
@@ -15,9 +15,7 @@
         <%= render "shared/login_to_comment" %>
       <% end %>
 
-      <% @comment_tree.root_comments.each do |comment| %>
-        <%= render "comments/comment", comment: comment %>
-      <% end %>
+      <%= render "comments/comment_list", comments: @comment_tree.root_comments %>
       <%= paginate @comment_tree.root_comments %>
     </div>
   </div>
diff --git a/app/views/legislation/proposals/_comments.html.erb b/app/views/legislation/proposals/_comments.html.erb
index 61efe69e2..a7471102d 100644
--- a/app/views/legislation/proposals/_comments.html.erb
+++ b/app/views/legislation/proposals/_comments.html.erb
@@ -10,9 +10,7 @@
         <%= render "shared/login_to_comment" %>
       <% end %>
 
-      <% @comment_tree.root_comments.each do |comment| %>
-        <%= render "comments/comment", comment: comment %>
-      <% end %>
+      <%= render "comments/comment_list", comments: @comment_tree.root_comments %>
       <%= paginate @comment_tree.root_comments %>
     </div>
   </div>
diff --git a/app/views/polls/_comments.html.erb b/app/views/polls/_comments.html.erb
index edab112a7..d3309d79d 100644
--- a/app/views/polls/_comments.html.erb
+++ b/app/views/polls/_comments.html.erb
@@ -10,9 +10,7 @@
         <%= render "shared/login_to_comment" %>
       <% end %>
 
-      <% @comment_tree.root_comments.each do |comment| %>
-        <%= render "comments/comment", comment: comment %>
-      <% end %>
+      <%= render "comments/comment_list", comments: @comment_tree.root_comments %>
       <%= paginate @comment_tree.root_comments %>
     </div>
   </div>
diff --git a/app/views/proposals/_comments.html.erb b/app/views/proposals/_comments.html.erb
index 61efe69e2..a7471102d 100644
--- a/app/views/proposals/_comments.html.erb
+++ b/app/views/proposals/_comments.html.erb
@@ -10,9 +10,7 @@
         <%= render "shared/login_to_comment" %>
       <% end %>
 
-      <% @comment_tree.root_comments.each do |comment| %>
-        <%= render "comments/comment", comment: comment %>
-      <% end %>
+      <%= render "comments/comment_list", comments: @comment_tree.root_comments %>
       <%= paginate @comment_tree.root_comments %>
     </div>
   </div>
diff --git a/app/views/topics/_comments.html.erb b/app/views/topics/_comments.html.erb
index e6e73e79e..485096378 100644
--- a/app/views/topics/_comments.html.erb
+++ b/app/views/topics/_comments.html.erb
@@ -4,10 +4,7 @@
     <div id="comments" class="small-12 column">
       <%= render "shared/wide_order_selector", i18n_namespace: "comments" %>
 
-      <% @comment_tree.root_comments.each do |comment| %>
-        <%= render "comments/comment", comment: comment %>
-      <% end %>
-
+      <%= render "comments/comment_list", comments: @comment_tree.root_comments %>
       <%= paginate @comment_tree.root_comments %>
 
       <% if user_signed_in? %>
diff --git a/spec/system/comments/budget_investments_spec.rb b/spec/system/comments/budget_investments_spec.rb
index 8e58c0773..3c922aee7 100644
--- a/spec/system/comments/budget_investments_spec.rb
+++ b/spec/system/comments/budget_investments_spec.rb
@@ -23,22 +23,22 @@ describe "Commenting Budget::Investments" do
   end
 
   scenario "Show" do
-    parent_comment = create(:comment, commentable: investment)
-    first_child    = create(:comment, commentable: investment, parent: parent_comment)
-    second_child   = create(:comment, commentable: investment, parent: parent_comment)
+    parent_comment = create(:comment, commentable: investment, body: "Parent")
+    create(:comment, commentable: investment, parent: parent_comment, body: "First subcomment")
+    create(:comment, commentable: investment, parent: parent_comment, body: "Last subcomment")
 
     visit comment_path(parent_comment)
 
     expect(page).to have_css(".comment", count: 3)
-    expect(page).to have_content parent_comment.body
-    expect(page).to have_content first_child.body
-    expect(page).to have_content second_child.body
+    expect(page).to have_content "Parent"
+    expect(page).to have_content "First subcomment"
+    expect(page).to have_content "Last subcomment"
 
     expect(page).to have_link "Go back to #{investment.title}", href: budget_investment_path(investment.budget, investment)
 
-    expect(page).to have_selector("ul#comment_#{parent_comment.id}>li", count: 2)
-    expect(page).to have_selector("ul#comment_#{first_child.id}>li", count: 1)
-    expect(page).to have_selector("ul#comment_#{second_child.id}>li", count: 1)
+    within ".comment", text: "Parent" do
+      expect(page).to have_selector(".comment", count: 2)
+    end
   end
 
   scenario "Link to comment show" do
diff --git a/spec/system/comments/debates_spec.rb b/spec/system/comments/debates_spec.rb
index 78af230b1..b9943c10c 100644
--- a/spec/system/comments/debates_spec.rb
+++ b/spec/system/comments/debates_spec.rb
@@ -20,22 +20,22 @@ describe "Commenting debates" do
   end
 
   scenario "Show" do
-    parent_comment = create(:comment, commentable: debate)
-    first_child    = create(:comment, commentable: debate, parent: parent_comment)
-    second_child   = create(:comment, commentable: debate, parent: parent_comment)
+    parent_comment = create(:comment, commentable: debate, body: "Parent")
+    create(:comment, commentable: debate, parent: parent_comment, body: "First subcomment")
+    create(:comment, commentable: debate, parent: parent_comment, body: "Last subcomment")
 
     visit comment_path(parent_comment)
 
     expect(page).to have_css(".comment", count: 3)
-    expect(page).to have_content parent_comment.body
-    expect(page).to have_content first_child.body
-    expect(page).to have_content second_child.body
+    expect(page).to have_content "Parent"
+    expect(page).to have_content "First subcomment"
+    expect(page).to have_content "Last subcomment"
 
     expect(page).to have_link "Go back to #{debate.title}", href: debate_path(debate)
 
-    expect(page).to have_selector("ul#comment_#{parent_comment.id}>li", count: 2)
-    expect(page).to have_selector("ul#comment_#{first_child.id}>li", count: 1)
-    expect(page).to have_selector("ul#comment_#{second_child.id}>li", count: 1)
+    within ".comment", text: "Parent" do
+      expect(page).to have_selector(".comment", count: 2)
+    end
   end
 
   scenario "Link to comment show" do
diff --git a/spec/system/comments/legislation_annotations_spec.rb b/spec/system/comments/legislation_annotations_spec.rb
index 4354da1f8..0ed8ef015 100644
--- a/spec/system/comments/legislation_annotations_spec.rb
+++ b/spec/system/comments/legislation_annotations_spec.rb
@@ -22,25 +22,25 @@ describe "Commenting legislation questions" do
   end
 
   scenario "Show" do
-    parent_comment = create(:comment, commentable: legislation_annotation)
-    first_child    = create(:comment, commentable: legislation_annotation, parent: parent_comment)
-    second_child   = create(:comment, commentable: legislation_annotation, parent: parent_comment)
     href           = legislation_process_draft_version_annotation_path(legislation_annotation.draft_version.process,
                                                                        legislation_annotation.draft_version,
                                                                        legislation_annotation)
+    parent_comment = create(:comment, commentable: legislation_annotation, body: "Parent")
+    create(:comment, commentable: legislation_annotation, parent: parent_comment, body: "First subcomment")
+    create(:comment, commentable: legislation_annotation, parent: parent_comment, body: "Last subcomment")
 
     visit comment_path(parent_comment)
 
     expect(page).to have_css(".comment", count: 3)
-    expect(page).to have_content parent_comment.body
-    expect(page).to have_content first_child.body
-    expect(page).to have_content second_child.body
+    expect(page).to have_content "Parent"
+    expect(page).to have_content "First subcomment"
+    expect(page).to have_content "Last subcomment"
 
     expect(page).to have_link "Go back to #{legislation_annotation.title}", href: href
 
-    expect(page).to have_selector("ul#comment_#{parent_comment.id}>li", count: 2)
-    expect(page).to have_selector("ul#comment_#{first_child.id}>li", count: 1)
-    expect(page).to have_selector("ul#comment_#{second_child.id}>li", count: 1)
+    within ".comment", text: "Parent" do
+      expect(page).to have_selector(".comment", count: 2)
+    end
   end
 
   scenario "Link to comment show" do
diff --git a/spec/system/comments/legislation_questions_spec.rb b/spec/system/comments/legislation_questions_spec.rb
index 09cc165dd..368c56190 100644
--- a/spec/system/comments/legislation_questions_spec.rb
+++ b/spec/system/comments/legislation_questions_spec.rb
@@ -25,23 +25,23 @@ describe "Commenting legislation questions" do
   end
 
   scenario "Show" do
-    parent_comment = create(:comment, commentable: legislation_question)
-    first_child    = create(:comment, commentable: legislation_question, parent: parent_comment)
-    second_child   = create(:comment, commentable: legislation_question, parent: parent_comment)
     href           = legislation_process_question_path(legislation_question.process, legislation_question)
+    parent_comment = create(:comment, commentable: legislation_question, body: "Parent")
+    create(:comment, commentable: legislation_question, parent: parent_comment, body: "First subcomment")
+    create(:comment, commentable: legislation_question, parent: parent_comment, body: "Last subcomment")
 
     visit comment_path(parent_comment)
 
     expect(page).to have_css(".comment", count: 3)
-    expect(page).to have_content parent_comment.body
-    expect(page).to have_content first_child.body
-    expect(page).to have_content second_child.body
+    expect(page).to have_content "Parent"
+    expect(page).to have_content "First subcomment"
+    expect(page).to have_content "Last subcomment"
 
     expect(page).to have_link "Go back to #{legislation_question.title}", href: href
 
-    expect(page).to have_selector("ul#comment_#{parent_comment.id}>li", count: 2)
-    expect(page).to have_selector("ul#comment_#{first_child.id}>li", count: 1)
-    expect(page).to have_selector("ul#comment_#{second_child.id}>li", count: 1)
+    within ".comment", text: "Parent" do
+      expect(page).to have_selector(".comment", count: 2)
+    end
   end
 
   scenario "Link to comment show" do
diff --git a/spec/system/comments/proposals_spec.rb b/spec/system/comments/proposals_spec.rb
index 84aa7c219..919e59a06 100644
--- a/spec/system/comments/proposals_spec.rb
+++ b/spec/system/comments/proposals_spec.rb
@@ -20,21 +20,21 @@ describe "Commenting proposals" do
   end
 
   scenario "Show" do
-    parent_comment = create(:comment, commentable: proposal)
-    first_child    = create(:comment, commentable: proposal, parent: parent_comment)
-    second_child   = create(:comment, commentable: proposal, parent: parent_comment)
+    parent_comment = create(:comment, commentable: proposal, body: "Parent")
+    create(:comment, commentable: proposal, parent: parent_comment, body: "First subcomment")
+    create(:comment, commentable: proposal, parent: parent_comment, body: "Last subcomment")
 
     visit comment_path(parent_comment)
 
     expect(page).to have_css(".comment", count: 3)
-    expect(page).to have_content parent_comment.body
-    expect(page).to have_content first_child.body
-    expect(page).to have_content second_child.body
+    expect(page).to have_content "Parent"
+    expect(page).to have_content "First subcomment"
+    expect(page).to have_content "Last subcomment"
     expect(page).to have_link "Go back to #{proposal.title}", href: proposal_path(proposal)
 
-    expect(page).to have_selector("ul#comment_#{parent_comment.id}>li", count: 2)
-    expect(page).to have_selector("ul#comment_#{first_child.id}>li", count: 1)
-    expect(page).to have_selector("ul#comment_#{second_child.id}>li", count: 1)
+    within ".comment", text: "Parent" do
+      expect(page).to have_selector(".comment", count: 2)
+    end
   end
 
   scenario "Link to comment show" do