From 7c6134fdeeae923b212824114dc205efb3db81cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Oct 2023 12:50:38 +0200 Subject: [PATCH] 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. --- .../stylesheets/documents/document.scss | 21 +++++++++++++++ .../stylesheets/documents/documents.scss | 4 --- app/assets/stylesheets/layout.scss | 8 ++---- app/assets/stylesheets/participation.scss | 15 +++++++++-- .../documents/document_component.html.erb | 26 +++++++------------ .../documents/document_component.rb | 6 +++-- .../documents/documents_component.html.erb | 4 ++- .../questions/read_more_component.html.erb | 6 +---- app/helpers/documents_helper.rb | 10 ------- .../documents/index.html.erb | 2 +- app/views/dashboard/_document.html.erb | 8 ------ app/views/dashboard/_proposed_action.html.erb | 4 ++- .../documents/_additional_document.html.erb | 3 --- .../documents/_additional_documents.html.erb | 4 ++- app/views/milestones/_milestone.html.erb | 10 +------ .../questions/read_more_component_spec.rb | 2 +- spec/shared/system/documentable.rb | 8 +++--- spec/shared/system/milestoneable.rb | 2 +- spec/shared/system/nested_documentable.rb | 4 +-- spec/system/admin/budget_investments_spec.rb | 3 +-- .../site_customization/documents_spec.rb | 4 +-- 21 files changed, 74 insertions(+), 80 deletions(-) create mode 100644 app/assets/stylesheets/documents/document.scss delete mode 100644 app/helpers/documents_helper.rb delete mode 100644 app/views/dashboard/_document.html.erb delete mode 100644 app/views/documents/_additional_document.html.erb diff --git a/app/assets/stylesheets/documents/document.scss b/app/assets/stylesheets/documents/document.scss new file mode 100644 index 000000000..47137d6c7 --- /dev/null +++ b/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; + } + } + } +} diff --git a/app/assets/stylesheets/documents/documents.scss b/app/assets/stylesheets/documents/documents.scss index d4cf19acb..6aab0c133 100644 --- a/app/assets/stylesheets/documents/documents.scss +++ b/app/assets/stylesheets/documents/documents.scss @@ -29,9 +29,5 @@ p { margin-bottom: 0; } - - a { - word-wrap: break-word; - } } } diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index b8ac99ee4..9a7b59f18 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -2028,12 +2028,8 @@ table { word-wrap: break-word; } - .icon-document { - color: #007bb7; - display: inline-block; - font-size: rem-calc(24); - line-height: $line-height; - vertical-align: middle; + > :first-child { + @include has-fa-icon(file, regular); } } diff --git a/app/assets/stylesheets/participation.scss b/app/assets/stylesheets/participation.scss index cca148e73..73fa7e58d 100644 --- a/app/assets/stylesheets/participation.scss +++ b/app/assets/stylesheets/participation.scss @@ -1442,8 +1442,19 @@ } } - .document-link a { - word-wrap: break-word; + .document-link { + > :first-child { + @include has-fa-icon(file, regular); + + &::before { + position: relative; + top: -0.1rem; + } + } + + a { + word-wrap: break-word; + } } } diff --git a/app/components/documents/document_component.html.erb b/app/components/documents/document_component.html.erb index a2a244933..2709de4b1 100644 --- a/app/components/documents/document_component.html.erb +++ b/app/components/documents/document_component.html.erb @@ -1,23 +1,17 @@ -
  • - <%= link_to t("documents.buttons.download_document"), - document.attachment, - target: "_blank", - rel: "nofollow", - class: "button hollow medium float-right" %> +
    + <%= link_to document.attachment, target: "_blank", rel: "nofollow" do %> + <%= document.title %> + + <%= document.humanized_content_type %> + <%= number_to_human_size(document.attachment_file_size, precision: 2) %> + + <% end %> - <%= document.title %> -
    - - <%= document.humanized_content_type %> |  - <%= number_to_human_size(document.attachment_file_size, precision: 2) %> - - - <% if can?(:destroy, document) %> -
    + <% if show_destroy_link? && can?(:destroy, document) %> <%= link_to t("documents.buttons.destroy_document"), document, method: :delete, data: { confirm: t("documents.actions.destroy.confirm") }, class: "delete" %> <% end %> -
    • + diff --git a/app/components/documents/document_component.rb b/app/components/documents/document_component.rb index ed418eda3..8f5c7f68a 100644 --- a/app/components/documents/document_component.rb +++ b/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 diff --git a/app/components/documents/documents_component.html.erb b/app/components/documents/documents_component.html.erb index d8912bd8c..ae30e464c 100644 --- a/app/components/documents/documents_component.html.erb +++ b/app/components/documents/documents_component.html.erb @@ -3,7 +3,9 @@ diff --git a/app/components/polls/questions/read_more_component.html.erb b/app/components/polls/questions/read_more_component.html.erb index 27f87d7ed..1a2fb9a89 100644 --- a/app/components/polls/questions/read_more_component.html.erb +++ b/app/components/polls/questions/read_more_component.html.erb @@ -30,15 +30,11 @@ <% if answer.documents.present? %>

    -   <%= t("polls.show.documents") %>

    <% answer.documents.each do |document| %> - <%= link_to document.title, - document.attachment, - target: "_blank", - rel: "nofollow" %>
    + <%= render Documents::DocumentComponent.new(document) %> <% end %>
    <% end %> diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb deleted file mode 100644 index 9110e45b8..000000000 --- a/app/helpers/documents_helper.rb +++ /dev/null @@ -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 diff --git a/app/views/admin/site_customization/documents/index.html.erb b/app/views/admin/site_customization/documents/index.html.erb index dd2cf7a60..85dd876e0 100644 --- a/app/views/admin/site_customization/documents/index.html.erb +++ b/app/views/admin/site_customization/documents/index.html.erb @@ -18,7 +18,7 @@ <% @documents.each do |document| %> - + <%= document.title %> <%= document.attachment.content_type %> <%= number_to_human_size(document.attachment.byte_size) %> diff --git a/app/views/dashboard/_document.html.erb b/app/views/dashboard/_document.html.erb deleted file mode 100644 index d59177a30..000000000 --- a/app/views/dashboard/_document.html.erb +++ /dev/null @@ -1,8 +0,0 @@ -

    - <%= link_to document.attachment, target: "_blank" do %> - <%= document.title %> - (<%= document.humanized_content_type %> -  |  - <%= number_to_human_size(document.attachment_file_size, precision: 2) %>) - <% end %> -

    diff --git a/app/views/dashboard/_proposed_action.html.erb b/app/views/dashboard/_proposed_action.html.erb index 12fa5b451..a19a047e1 100644 --- a/app/views/dashboard/_proposed_action.html.erb +++ b/app/views/dashboard/_proposed_action.html.erb @@ -49,7 +49,9 @@

    <%= link_to link.label, link.url, target: "_blank" %>

    <% end %> - <%= render partial: "document", collection: proposed_action.documents %> + <% proposed_action.documents.each do |document| %> + <%= render Documents::DocumentComponent.new(document) %> + <% end %> diff --git a/app/views/documents/_additional_document.html.erb b/app/views/documents/_additional_document.html.erb deleted file mode 100644 index 8c079590e..000000000 --- a/app/views/documents/_additional_document.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -

      -<%= document_item_link(document) %> -

    diff --git a/app/views/documents/_additional_documents.html.erb b/app/views/documents/_additional_documents.html.erb index b4b23a1a0..225147f4b 100644 --- a/app/views/documents/_additional_documents.html.erb +++ b/app/views/documents/_additional_documents.html.erb @@ -6,7 +6,9 @@

    <%= t("documents.additional") %>

    - <%= render partial: "documents/additional_document", collection: documents, as: :document %> + <% documents.each do |document| %> + <%= render Documents::DocumentComponent.new(document) %> + <% end %> diff --git a/app/views/milestones/_milestone.html.erb b/app/views/milestones/_milestone.html.erb index 79b5fdf99..54371534a 100644 --- a/app/views/milestones/_milestone.html.erb +++ b/app/views/milestones/_milestone.html.erb @@ -36,15 +36,7 @@

    <% milestone.documents.each do |document| %> - <%= link_to document.title, - document.attachment, - target: "_blank", - rel: "nofollow" %>
    - - <%= document.humanized_content_type %> |  - <%= number_to_human_size(document.attachment_file_size, precision: 2) %> - -
    + <%= render Documents::DocumentComponent.new(document) %> <% end %> diff --git a/spec/components/polls/questions/read_more_component_spec.rb b/spec/components/polls/questions/read_more_component_spec.rb index 053bc5097..26c9899ca 100644 --- a/spec/components/polls/questions/read_more_component_spec.rb +++ b/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 diff --git a/spec/shared/system/documentable.rb b/spec/shared/system/documentable.rb index 83b2624b7..d4eda590e 100644 --- a/spec/shared/system/documentable.rb +++ b/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" - expect(page).to have_selector "a[target=_blank]", text: "Download file" - expect(page).to have_selector "a[rel=nofollow]", text: "Download file" + within "#documents" do + expect(page).to have_link text: document.title + 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 diff --git a/spec/shared/system/milestoneable.rb b/spec/shared/system/milestoneable.rb index 7fc8f653f..13569e63f 100644 --- a/spec/shared/system/milestoneable.rb +++ b/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 diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index f73ab60e6..441330b57 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/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 diff --git a/spec/system/admin/budget_investments_spec.rb b/spec/system/admin/budget_investments_spec.rb index ac7408eee..506562d17 100644 --- a/spec/system/admin/budget_investments_spec.rb +++ b/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_content("Download file") + expect(page).to have_link text: document.title expect(page).to have_content("1234") expect(page).to have_content("1000") expect(page).to have_content("Unfeasible") diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index c1b09057c..ae65fe484 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/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