Unify the way we display document information

We were displaying documents in five places, and in five different ways.
Sometimes with the metadata in parenthesis after the title, sometimes
with the metadata below the title, sometimes without metadata, sometimes
with an icon in front of the document, and sometimes with a separate
link to download the file.

So we're now displaying the same thing everywhere. Not sure whether this
is the best solution, but at least it's consistent.

We aren't unifying the way we display a list of documents, though, since
different sections look pretty different and I'm not sure whether the
same style would look well everywhere.

Note that we're renaming the `document` HTML class in the documents
table to `document-row` so the styles for the `document` class don't
apply here.

app/assets/stylesheets/documents/document.scss

@@ -0,0 +1,21 @@
+.document {
+  + .document {
+    margin-top: $line-height / 3;
+  }
+
+  a:first-of-type {
+    word-wrap: break-word;
+
+    .document-metadata {
+      &::before,
+      &::after {
+        content: "";
+        display: block;
+      }
+
+      * + *::before {
+        @include vertical-separator;
+      }
+    }
+  }
+}

app/assets/stylesheets/documents/documents.scss

@@ -29,9 +29,5 @@
     p {
       margin-bottom: 0;
     }
-
-    a {
-      word-wrap: break-word;
-    }
   }
 }

app/assets/stylesheets/layout.scss

@@ -2028,12 +2028,8 @@ table {
     word-wrap: break-word;
   }
 
-  .icon-document {
+  > :first-child {
-    color: #007bb7;
+    @include has-fa-icon(file, regular);
-    display: inline-block;
-    font-size: rem-calc(24);
-    line-height: $line-height;
-    vertical-align: middle;
   }
 }
 

app/assets/stylesheets/participation.scss

@@ -1442,8 +1442,19 @@
     }
   }
 
-  .document-link a {
+  .document-link {
-    word-wrap: break-word;
+    > :first-child {
+      @include has-fa-icon(file, regular);
+
+      &::before {
+        position: relative;
+        top: -0.1rem;
+      }
+    }
+
+    a {
+      word-wrap: break-word;
+    }
   }
 }
 

app/components/documents/document_component.html.erb

@@ -1,23 +1,17 @@
-<li id="<%= dom_id(document) %>">
+<div id="<%= dom_id(document) %>" class="document">
-  <%= link_to t("documents.buttons.download_document"),
+  <%= link_to document.attachment, target: "_blank", rel: "nofollow" do %>
-              document.attachment,
+    <strong class="document-title"><%= document.title %></strong>
-              target: "_blank",
+    <small class="document-metadata">
-              rel: "nofollow",
+      <span class="document-content-type"><%= document.humanized_content_type %></span>
-              class: "button hollow medium float-right" %>
+      <span class="document-size"><%= number_to_human_size(document.attachment_file_size, precision: 2) %></span>
+    </small>
+  <% end %>
 
-  <strong><%= document.title %></strong>
+  <% if show_destroy_link? && can?(:destroy, document) %>
-  <br>
-  <small>
-    <%= document.humanized_content_type %>&nbsp;|&nbsp;
-    <%= number_to_human_size(document.attachment_file_size, precision: 2) %>
-  </small>
-
-  <% if can?(:destroy, document) %>
-    <br>
     <%= link_to t("documents.buttons.destroy_document"),
                 document,
                 method: :delete,
                 data: { confirm: t("documents.actions.destroy.confirm") },
                 class: "delete" %>
   <% end %>
-</li>
+</div>

app/components/documents/document_component.rb

@@ -1,8 +1,10 @@
 class Documents::DocumentComponent < ApplicationComponent
-  attr_reader :document
+  attr_reader :document, :show_destroy_link
+  alias_method :show_destroy_link?, :show_destroy_link
   delegate :can?, to: :helpers
 
-  def initialize(document)
+  def initialize(document, show_destroy_link: false)
     @document = document
+    @show_destroy_link = show_destroy_link
   end
 end

app/components/documents/documents_component.html.erb

@@ -3,7 +3,9 @@
 
   <ul class="no-bullet document-link">
     <% documents.each do |document| %>
-      <%= render Documents::DocumentComponent.new(document) %>
+      <li>
+        <%= render Documents::DocumentComponent.new(document, show_destroy_link: true) %>
+      </li>
     <% end %>
   </ul>
 </div>

app/components/polls/questions/read_more_component.html.erb

@@ -30,15 +30,11 @@
       <% if answer.documents.present? %>
         <div class="document-link">
           <p>
-            <span class="icon-document"></span>&nbsp;
             <strong><%= t("polls.show.documents") %></strong>
           </p>
 
           <% answer.documents.each do |document| %>
-              <%= link_to document.title,
+            <%= render Documents::DocumentComponent.new(document) %>
-                          document.attachment,
-                          target: "_blank",
-                          rel: "nofollow" %><br>
           <% end %>
         </div>
       <% end %>

app/helpers/documents_helper.rb

@@ -1,10 +0,0 @@
-module DocumentsHelper
-  def document_item_link(document)
-    info_text = "#{document.humanized_content_type} | #{number_to_human_size(document.attachment_file_size)}"
-
-    link_to safe_join([document.title, tag.small("(#{info_text})")], " "),
-            document.attachment,
-            target: "_blank",
-            title: t("shared.target_blank")
-  end
-end

app/views/admin/site_customization/documents/index.html.erb

@@ -18,7 +18,7 @@
     </thead>
     <tbody id="documents">
       <% @documents.each do |document| %>
-        <tr id="<%= dom_id(document) %>" class="document">
+        <tr id="<%= dom_id(document) %>" class="document-row">
           <td><%= document.title %></td>
           <td><%= document.attachment.content_type %></td>
           <td><%= number_to_human_size(document.attachment.byte_size) %></td>

app/views/dashboard/_document.html.erb

@@ -1,8 +0,0 @@
-<p>
-  <%= link_to document.attachment, target: "_blank" do %>
-    <%= document.title %>
-    (<%= document.humanized_content_type %>
-    &nbsp;|&nbsp;
-    <%= number_to_human_size(document.attachment_file_size, precision: 2) %>)
-  <% end %>
-</p>

app/views/dashboard/_proposed_action.html.erb

@@ -49,7 +49,9 @@
         <p><%= link_to link.label, link.url, target: "_blank" %></p>
       <% end %>
 
-      <%= render partial: "document", collection: proposed_action.documents %>
+      <% proposed_action.documents.each do |document| %>
+        <%= render Documents::DocumentComponent.new(document) %>
+      <% end %>
     </div>
   </div>
 </div>

app/views/documents/_additional_document.html.erb

@@ -1,3 +0,0 @@
-<p><span class="icon-document"></span>&nbsp;
-<%= document_item_link(document) %>
-</p>

app/views/documents/_additional_documents.html.erb

@@ -6,7 +6,9 @@
           <p>
             <strong><%= t("documents.additional") %></strong>
           </p>
-          <%= render partial: "documents/additional_document", collection: documents, as: :document %>
+          <% documents.each do |document| %>
+            <%= render Documents::DocumentComponent.new(document) %>
+          <% end %>
         </div>
       </div>
     </div>

app/views/milestones/_milestone.html.erb

@@ -36,15 +36,7 @@
           </p>
 
           <% milestone.documents.each do |document| %>
-              <%= link_to document.title,
+            <%= render Documents::DocumentComponent.new(document) %>
-                          document.attachment,
-                          target: "_blank",
-                          rel: "nofollow" %><br>
-              <small>
-                <%= document.humanized_content_type %>&nbsp;|&nbsp;
-                <%= number_to_human_size(document.attachment_file_size, precision: 2) %>
-              </small>
-              <br>
           <% end %>
         </div>
       </div>

spec/components/polls/questions/read_more_component_spec.rb

@@ -52,6 +52,6 @@ describe Polls::Questions::ReadMoreComponent do
 
     render_inline Polls::Questions::ReadMoreComponent.new(question: question)
 
-    expect(page).to have_link("The yes movement")
+    expect(page).to have_link text: "The yes movement"
   end
 end

spec/shared/system/documentable.rb

@@ -14,9 +14,11 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path,
     scenario "Download action should be availabe to anyone and open in a new window" do
       visit send(documentable_path, arguments)
 
-      expect(page).to have_link "Download file"
+      within "#documents" do
-      expect(page).to have_selector "a[target=_blank]", text: "Download file"
+        expect(page).to have_link text: document.title
-      expect(page).to have_selector "a[rel=nofollow]", text: "Download file"
+        expect(page).to have_selector "a[target=_blank]", text: document.title
+        expect(page).to have_selector "a[rel=nofollow]", text: document.title
+      end
     end
 
     describe "Destroy action" do

spec/shared/system/milestoneable.rb

@@ -29,7 +29,7 @@ shared_examples "milestoneable" do |factory_name|
         expect(page).to have_content(Date.yesterday)
         expect(page).not_to have_content(Date.current)
         expect(page.find("#image_#{first_milestone.id}")["alt"]).to have_content(image.title)
-        expect(page).to have_link(document.title)
+        expect(page).to have_link text: document.title
         expect(page).to have_link("https://consul.dev")
         expect(page).to have_content(first_milestone.status.name)
       end

spec/shared/system/nested_documentable.rb

@@ -291,14 +291,14 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na
 
         expect(page).to have_content documentable_success_notice
 
-        original_url = find_link("Download file")[:href]
+        original_url = find_link(text: "Original")[:href]
 
         visit send(path, arguments)
         within_fieldset("Documents") { fill_in "Title", with: "Updated" }
         click_button submit_button
 
         expect(page).to have_content documentable_success_notice
-        expect(find_link("Download file")[:href]).to eq original_url
+        expect(find_link(text: "Updated")[:href]).to eq original_url
       end
     end
 

spec/system/admin/budget_investments_spec.rb

@@ -981,8 +981,7 @@ describe "Admin budget investments", :admin do
         expect(page).to have_content("Investment preview")
         expect(page).to have_content(budget_investment.image.title)
         expect(page).to have_content("Documents (1)")
-        expect(page).to have_content(document.title)
+        expect(page).to have_link text: document.title
-        expect(page).to have_content("Download file")
         expect(page).to have_content("1234")
         expect(page).to have_content("1000")
         expect(page).to have_content("Unfeasible")

spec/system/admin/site_customization/documents_spec.rb

@@ -39,7 +39,7 @@ describe "Documents", :admin do
 
     visit admin_site_customization_documents_path
 
-    expect(page).to have_selector("#documents .document", count: per_page)
+    expect(page).to have_selector("#documents .document-row", count: per_page)
 
     within("ul.pagination") do
       expect(page).to have_content("1")
@@ -48,7 +48,7 @@ describe "Documents", :admin do
       click_link "Next", exact: false
     end
 
-    expect(page).to have_selector("#documents .document", count: 2)
+    expect(page).to have_selector("#documents .document-row", count: 2)
   end
 
   scenario "Create" do