From 1826e3b6914cd54a84c3e45c68b37464b71b4828 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 16:16:36 +0200 Subject: [PATCH 01/14] Remove legacy Paperclip columns We haven't used these columns since commit 7212657c0, and every Consul Democracy installation has been using Active Storage since version 1.5.0. --- .../20231012141318_remove_paperclip_columns.rb | 17 +++++++++++++++++ db/schema.rb | 10 +--------- 2 files changed, 18 insertions(+), 9 deletions(-) create mode 100644 db/migrate/20231012141318_remove_paperclip_columns.rb diff --git a/db/migrate/20231012141318_remove_paperclip_columns.rb b/db/migrate/20231012141318_remove_paperclip_columns.rb new file mode 100644 index 000000000..cd1daab4b --- /dev/null +++ b/db/migrate/20231012141318_remove_paperclip_columns.rb @@ -0,0 +1,17 @@ +class RemovePaperclipColumns < ActiveRecord::Migration[6.1] + def change + change_table :images do |t| + t.remove :attachment_file_name, type: :string + t.remove :attachment_content_type, type: :string + t.remove :attachment_file_size, type: :bigint + t.remove :attachment_updated_at, type: :datetime + end + + change_table :documents do |t| + t.remove :attachment_file_name, type: :string + t.remove :attachment_content_type, type: :string + t.remove :attachment_file_size, type: :bigint + t.remove :attachment_updated_at, type: :datetime + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 11ebacaa9..603deabd5 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2023_05_23_090028) do +ActiveRecord::Schema.define(version: 2023_10_12_141318) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -585,10 +585,6 @@ ActiveRecord::Schema.define(version: 2023_05_23_090028) do create_table "documents", id: :serial, force: :cascade do |t| t.string "title" - t.string "attachment_file_name" - t.string "attachment_content_type" - t.bigint "attachment_file_size" - t.datetime "attachment_updated_at" t.integer "user_id" t.string "documentable_type" t.integer "documentable_id" @@ -684,10 +680,6 @@ ActiveRecord::Schema.define(version: 2023_05_23_090028) do t.string "title", limit: 80 t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "attachment_file_name" - t.string "attachment_content_type" - t.bigint "attachment_file_size" - t.datetime "attachment_updated_at" t.integer "user_id" t.index ["imageable_type", "imageable_id"], name: "index_images_on_imageable_type_and_imageable_id" t.index ["user_id"], name: "index_images_on_user_id" From bdb92e41616784be140ee98834ce5f68b9235c5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 16:26:44 +0200 Subject: [PATCH 02/14] Remove redundant link to download document There's a link next to it that does the exact same thing and includes the word "download", which was confusing in some cases since people might think that links with different texts lead to different pages. --- .../questions/answers/documents/index_component.html.erb | 2 +- .../poll/questions/answers/documents/documents_spec.rb | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/app/components/admin/poll/questions/answers/documents/index_component.html.erb b/app/components/admin/poll/questions/answers/documents/index_component.html.erb index 6db626293..b23f46ef2 100644 --- a/app/components/admin/poll/questions/answers/documents/index_component.html.erb +++ b/app/components/admin/poll/questions/answers/documents/index_component.html.erb @@ -36,7 +36,7 @@ <% documents.each do |document| %> - <%= link_to document.title, document.attachment %> + <%= document.title %> <%= render Admin::Poll::Questions::Answers::Documents::TableActionsComponent.new(document) %> diff --git a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb index cc6f55bc9..9a65fc182 100644 --- a/spec/system/admin/poll/questions/answers/documents/documents_spec.rb +++ b/spec/system/admin/poll/questions/answers/documents/documents_spec.rb @@ -30,11 +30,16 @@ describe "Documents", :admin do visit admin_answer_documents_path(answer) + expect(page).not_to have_link "Download file" + documentable_attach_new_file(Rails.root.join("spec/fixtures/files/clippy.pdf")) click_button "Save" expect(page).to have_content "Document uploaded successfully" - expect(page).to have_link "clippy.pdf" + + within("tr", text: "clippy.pdf") do + expect(page).to have_link "Download file" + end end scenario "with invalid data" do From 6d59a847ebc1d1f630de3f760e552530692af36b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 17:18:43 +0200 Subject: [PATCH 03/14] Unify links to download documents in admin tables We were using a "Download file" link in one place, while in another place we had an additional column where the name of the document was a link to download it. --- .../documents/index.html.erb | 20 ++++++++++--------- config/locales/en/admin.yml | 1 - config/locales/es/admin.yml | 1 - .../site_customization/documents_spec.rb | 5 +++-- 4 files changed, 14 insertions(+), 13 deletions(-) diff --git a/app/views/admin/site_customization/documents/index.html.erb b/app/views/admin/site_customization/documents/index.html.erb index 4f3f93497..dd2cf7a60 100644 --- a/app/views/admin/site_customization/documents/index.html.erb +++ b/app/views/admin/site_customization/documents/index.html.erb @@ -13,7 +13,6 @@ <%= t("admin.shared.title") %> <%= t("admin.documents.index.format") %> <%= t("admin.documents.index.size") %> - <%= t("admin.documents.index.url") %> <%= t("admin.shared.actions") %> @@ -23,15 +22,18 @@ <%= document.title %> <%= document.attachment.content_type %> <%= number_to_human_size(document.attachment.byte_size) %> - <%= link_to document.title, document.attachment, target: :blank %> -
- <%= render Admin::TableActionsComponent.new( - document, - actions: [:destroy], - destroy_path: admin_site_customization_document_path(document) - ) %> -
+ <%= render Admin::TableActionsComponent.new( + document, + actions: [:destroy], + destroy_path: admin_site_customization_document_path(document) + ) do |actions| %> + <%= actions.action(:download, + text: t("documents.buttons.download_document"), + path: document.attachment, + target: "_blank", + rel: "nofollow") %> + <% end %> <% end %> diff --git a/config/locales/en/admin.yml b/config/locales/en/admin.yml index 031a9759c..e28b06f0c 100644 --- a/config/locales/en/admin.yml +++ b/config/locales/en/admin.yml @@ -529,7 +529,6 @@ en: title: "Documents" format: "Format" size: "Size" - url: "URL" create: success_notice: "Document uploaded successfully" unable_notice: "Invalid document" diff --git a/config/locales/es/admin.yml b/config/locales/es/admin.yml index 1c317d3a3..c25c6436f 100644 --- a/config/locales/es/admin.yml +++ b/config/locales/es/admin.yml @@ -529,7 +529,6 @@ es: title: "Documentos" format: "Formato" size: "Tamaño" - url: "URL" create: success_notice: "Documento creado correctamente" unable_notice: "Documento inválido" diff --git a/spec/system/admin/site_customization/documents_spec.rb b/spec/system/admin/site_customization/documents_spec.rb index 5c284c525..c1b09057c 100644 --- a/spec/system/admin/site_customization/documents_spec.rb +++ b/spec/system/admin/site_customization/documents_spec.rb @@ -23,7 +23,7 @@ describe "Documents", :admin do visit admin_site_customization_documents_path expect(page).to have_content "There are 3 documents" - expect(page).to have_link document.title, href: url + expect(page).to have_link "Download file", href: url end scenario "Index (empty)" do @@ -58,7 +58,8 @@ describe "Documents", :admin do click_button "Upload" expect(page).to have_content "Document uploaded successfully" - expect(page).to have_link "logo.pdf" + + within("tr", text: "logo.pdf") { expect(page).to have_link "Download file" } end scenario "Errors on create" do From 6d8f6445bcb02b703e8df32b24a6ca957cac30d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 18:18:31 +0200 Subject: [PATCH 04/14] Remove unused parameter when rendering documents This parameter isn't used in the `documents/_documents` partial. --- app/views/budgets/investments/_investment_detail.html.erb | 4 +--- app/views/legislation/proposals/show.html.erb | 4 +--- app/views/proposals/_info.html.erb | 4 +--- 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/app/views/budgets/investments/_investment_detail.html.erb b/app/views/budgets/investments/_investment_detail.html.erb index 626cabf8f..cab44899d 100644 --- a/app/views/budgets/investments/_investment_detail.html.erb +++ b/app/views/budgets/investments/_investment_detail.html.erb @@ -45,9 +45,7 @@ <% end %> <% if feature?(:allow_attached_documents) %> - <%= render "documents/documents", - documents: investment.documents, - max_documents_allowed: Budget::Investment.max_documents_allowed %> + <%= render "documents/documents", documents: investment.documents %> <% end %> <%= render "shared/tags", taggable: investment %> diff --git a/app/views/legislation/proposals/show.html.erb b/app/views/legislation/proposals/show.html.erb index 406e8fa36..ec31d0f88 100644 --- a/app/views/legislation/proposals/show.html.erb +++ b/app/views/legislation/proposals/show.html.erb @@ -87,9 +87,7 @@ <% end %> <% if feature?(:allow_attached_documents) %> - <%= render "documents/documents", - documents: @proposal.documents, - max_documents_allowed: Proposal.max_documents_allowed %> + <%= render "documents/documents", documents: @proposal.documents %> <% end %> <%= render "shared/tags", taggable: @proposal %> diff --git a/app/views/proposals/_info.html.erb b/app/views/proposals/_info.html.erb index ec5d2b866..6bb723136 100644 --- a/app/views/proposals/_info.html.erb +++ b/app/views/proposals/_info.html.erb @@ -67,9 +67,7 @@ <% end %> <% if feature?(:allow_attached_documents) %> - <%= render "documents/documents", - documents: @proposal.documents, - max_documents_allowed: Proposal.max_documents_allowed %> + <%= render "documents/documents", documents: @proposal.documents %> <% end %> <%= render "shared/tags", taggable: @proposal %> From 2093083d29a0757274ade5a036f6fa6c6bcd9bb4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 21:05:41 +0200 Subject: [PATCH 05/14] Remove
tags in document fields We aren't using
tags on any forms containing fields to add/edit documents, so using this in the dashboard actions form and the legislation process form was inconsistent. --- app/views/admin/dashboard/actions/_form.html.erb | 3 --- app/views/admin/legislation/processes/_form.html.erb | 12 ------------ 2 files changed, 15 deletions(-) diff --git a/app/views/admin/dashboard/actions/_form.html.erb b/app/views/admin/dashboard/actions/_form.html.erb index d98bd9501..f479f6ad7 100644 --- a/app/views/admin/dashboard/actions/_form.html.erb +++ b/app/views/admin/dashboard/actions/_form.html.erb @@ -65,15 +65,12 @@ <% if feature?(:allow_attached_documents) %>
-
<%= render "documents/nested_documents", f: f %> -
<% end %>
<%= render "links/nested_links", linkable: dashboard_action, f: f %> -
diff --git a/app/views/admin/legislation/processes/_form.html.erb b/app/views/admin/legislation/processes/_form.html.erb index 3094d4ccd..0fe41f941 100644 --- a/app/views/admin/legislation/processes/_form.html.erb +++ b/app/views/admin/legislation/processes/_form.html.erb @@ -113,18 +113,10 @@ <%= render "documents/nested_documents", f: f %>
-
-
-
-
<%= render "images/nested_image", f: f %>
-
-
-
-

<%= t("admin.legislation.processes.form.banner_title") %>

@@ -154,10 +146,6 @@ - -
-
-
From f9bb178cb043b67913cbdd3720640856f3b953af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 21:16:38 +0200 Subject: [PATCH 06/14] Remove obsolete data attribute This attribute isn't used since commit 88a7a29d2. --- app/views/legislation/proposals/_form.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/legislation/proposals/_form.html.erb b/app/views/legislation/proposals/_form.html.erb index 7fa3d3a4b..69a699bf5 100644 --- a/app/views/legislation/proposals/_form.html.erb +++ b/app/views/legislation/proposals/_form.html.erb @@ -32,7 +32,7 @@
<% end %> -
+
<%= render "documents/nested_documents", f: f %>
From 00f063b291322625759d1dbb7d5b1c65cc0a13fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 21:18:04 +0200 Subject: [PATCH 07/14] Remove unused documents-related CSS We don't have any elements matching the `.documents .icon-document` selector. --- app/assets/stylesheets/layout.scss | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index fc211c76a..2710af584 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -2044,14 +2044,6 @@ table { padding: $line-height / 2; position: relative; - .icon-document { - color: #007bb7; - display: inline-block; - font-size: rem-calc(24); - line-height: $line-height; - vertical-align: middle; - } - p { margin-bottom: 0; } From 1e1d7996bb45b7dbfae6fb284c2efb396bd72926 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 21:21:42 +0200 Subject: [PATCH 08/14] Simplify rendering document/image fields We were adding
tags with the `images` or `documents` HTML class prettly much every time we rendered a NestedComponent. We're now including the HTML class inside the component, as we usually do. We're also rendering the nested components directly, since it's been a while since the partials were changed to simply render the components. --- .../admin/budget_phases/form_component.html.erb | 4 ++-- app/components/admin/budgets/form_component.html.erb | 4 ++-- .../questions/answers/documents/index_component.html.erb | 4 +--- .../budgets/investments/form_component.html.erb | 8 ++------ app/components/documents/nested_component.html.erb | 2 +- app/components/images/nested_component.html.erb | 2 +- app/components/proposals/form_component.html.erb | 8 ++------ app/views/admin/dashboard/actions/_form.html.erb | 4 ++-- app/views/admin/legislation/processes/_form.html.erb | 8 ++++---- app/views/admin/milestones/_form.html.erb | 7 ++----- app/views/admin/poll/polls/_form.html.erb | 4 ++-- .../admin/poll/questions/answers/images/new.html.erb | 4 +--- app/views/admin/widget/cards/_form.html.erb | 2 +- app/views/documents/_nested_documents.html.erb | 1 - app/views/images/_nested_image.html.erb | 1 - app/views/legislation/proposals/_form.html.erb | 8 ++++---- 16 files changed, 27 insertions(+), 44 deletions(-) delete mode 100644 app/views/documents/_nested_documents.html.erb delete mode 100644 app/views/images/_nested_image.html.erb diff --git a/app/components/admin/budget_phases/form_component.html.erb b/app/components/admin/budget_phases/form_component.html.erb index 369373454..d9b494220 100644 --- a/app/components/admin/budget_phases/form_component.html.erb +++ b/app/components/admin/budget_phases/form_component.html.erb @@ -56,8 +56,8 @@ <% end %> <% if feature?(:allow_images) %> -
- <%= render "images/nested_image", f: f %> +
+ <%= render Images::NestedComponent.new(f) %>

<%= t("admin.budget_phases.edit.image_description") %>

<% end %> diff --git a/app/components/admin/budgets/form_component.html.erb b/app/components/admin/budgets/form_component.html.erb index 38cc4e6f6..0f958d4d9 100644 --- a/app/components/admin/budgets/form_component.html.erb +++ b/app/components/admin/budgets/form_component.html.erb @@ -54,8 +54,8 @@ <% end %> <% if feature?(:allow_images) %> -
- <%= render "/images/nested_image", f: f %> +
+ <%= render Images::NestedComponent.new(f) %>

<%= t("admin.budgets.edit.image_description") %>

<% end %> diff --git a/app/components/admin/poll/questions/answers/documents/index_component.html.erb b/app/components/admin/poll/questions/answers/documents/index_component.html.erb index b23f46ef2..627159982 100644 --- a/app/components/admin/poll/questions/answers/documents/index_component.html.erb +++ b/app/components/admin/poll/questions/answers/documents/index_component.html.erb @@ -12,9 +12,7 @@ <%= form_for(Poll::Question::Answer.new, url: admin_answer_documents_path(answer)) do |f| %> <%= render "shared/errors", resource: answer %> -
- <%= render "documents/nested_documents", f: f %> -
+ <%= render Documents::NestedComponent.new(f) %>
<%= f.submit(class: "button expanded", value: t("shared.save")) %> diff --git a/app/components/budgets/investments/form_component.html.erb b/app/components/budgets/investments/form_component.html.erb index daa0aec4a..228aec322 100644 --- a/app/components/budgets/investments/form_component.html.erb +++ b/app/components/budgets/investments/form_component.html.erb @@ -37,15 +37,11 @@ <%= t("shared.optional") %> <% if feature?(:allow_images) %> -
- <%= render "images/nested_image", f: f %> -
+ <%= render Images::NestedComponent.new(f) %> <% end %> <% if feature?(:allow_attached_documents) %> -
- <%= render "documents/nested_documents", f: f %> -
+ <%= render Documents::NestedComponent.new(f) %> <% end %> <% if feature?(:map) %> diff --git a/app/components/documents/nested_component.html.erb b/app/components/documents/nested_component.html.erb index 743cd7be3..d47185ae8 100644 --- a/app/components/documents/nested_component.html.erb +++ b/app/components/documents/nested_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= t("documents.form.title") %>

<%= note %>

diff --git a/app/components/images/nested_component.html.erb b/app/components/images/nested_component.html.erb index 0505ea4de..578cb53fe 100644 --- a/app/components/images/nested_component.html.erb +++ b/app/components/images/nested_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= t("images.form.title") %>

<%= note %>

diff --git a/app/components/proposals/form_component.html.erb b/app/components/proposals/form_component.html.erb index f6084f3b8..fddca56bb 100644 --- a/app/components/proposals/form_component.html.erb +++ b/app/components/proposals/form_component.html.erb @@ -38,15 +38,11 @@
<% if feature?(:allow_images) %> -
- <%= render "images/nested_image", f: f %> -
+ <%= render Images::NestedComponent.new(f) %> <% end %> <% if feature?(:allow_attached_documents) %> -
- <%= render "documents/nested_documents", f: f %> -
+ <%= render Documents::NestedComponent.new(f) %> <% end %> <% if Geozone.any? %> diff --git a/app/views/admin/dashboard/actions/_form.html.erb b/app/views/admin/dashboard/actions/_form.html.erb index f479f6ad7..3dd147172 100644 --- a/app/views/admin/dashboard/actions/_form.html.erb +++ b/app/views/admin/dashboard/actions/_form.html.erb @@ -64,8 +64,8 @@
<% if feature?(:allow_attached_documents) %> -
- <%= render "documents/nested_documents", f: f %> +
+ <%= render Documents::NestedComponent.new(f) %>
<% end %> diff --git a/app/views/admin/legislation/processes/_form.html.erb b/app/views/admin/legislation/processes/_form.html.erb index 0fe41f941..0f461bfd7 100644 --- a/app/views/admin/legislation/processes/_form.html.erb +++ b/app/views/admin/legislation/processes/_form.html.erb @@ -109,12 +109,12 @@
-
- <%= render "documents/nested_documents", f: f %> +
+ <%= render Documents::NestedComponent.new(f) %>
-
- <%= render "images/nested_image", f: f %> +
+ <%= render Images::NestedComponent.new(f) %>
diff --git a/app/views/admin/milestones/_form.html.erb b/app/views/admin/milestones/_form.html.erb index 7198f1c93..2d7d6b2a1 100644 --- a/app/views/admin/milestones/_form.html.erb +++ b/app/views/admin/milestones/_form.html.erb @@ -27,11 +27,8 @@
<%= f.date_field :publication_date %> - <%= render "images/nested_image", f: f %> - -
- <%= render "documents/nested_documents", f: f %> -
+ <%= render Images::NestedComponent.new(f) %> + <%= render Documents::NestedComponent.new(f) %> <%= f.submit nil, class: "button success" %>
diff --git a/app/views/admin/poll/polls/_form.html.erb b/app/views/admin/poll/polls/_form.html.erb index dd0b59292..332011a20 100644 --- a/app/views/admin/poll/polls/_form.html.erb +++ b/app/views/admin/poll/polls/_form.html.erb @@ -32,8 +32,8 @@
-
- <%= render "images/nested_image", f: f %> +
+ <%= render Images::NestedComponent.new(f) %>
diff --git a/app/views/admin/poll/questions/answers/images/new.html.erb b/app/views/admin/poll/questions/answers/images/new.html.erb index e4ddaa769..3c85c0b57 100644 --- a/app/views/admin/poll/questions/answers/images/new.html.erb +++ b/app/views/admin/poll/questions/answers/images/new.html.erb @@ -2,9 +2,7 @@ <%= form_for(@answer, url: admin_answer_images_path(@answer), method: :post) do |f| %> <%= render "shared/errors", resource: @answer %> -
- <%= render Images::NestedComponent.new(f, image_fields: :images) %> -
+ <%= render Images::NestedComponent.new(f, image_fields: :images) %> <%= f.submit t("admin.questions.answers.images.save_image"), class: "button success" %> <% end %> diff --git a/app/views/admin/widget/cards/_form.html.erb b/app/views/admin/widget/cards/_form.html.erb index 606697f4d..d01dc0cc1 100644 --- a/app/views/admin/widget/cards/_form.html.erb +++ b/app/views/admin/widget/cards/_form.html.erb @@ -45,7 +45,7 @@
- <%= render "images/nested_image", f: f %> + <%= render Images::NestedComponent.new(f) %>
diff --git a/app/views/documents/_nested_documents.html.erb b/app/views/documents/_nested_documents.html.erb deleted file mode 100644 index 229840e83..000000000 --- a/app/views/documents/_nested_documents.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= render Documents::NestedComponent.new(f) %> diff --git a/app/views/images/_nested_image.html.erb b/app/views/images/_nested_image.html.erb deleted file mode 100644 index c55ccaa56..000000000 --- a/app/views/images/_nested_image.html.erb +++ /dev/null @@ -1 +0,0 @@ -<%= render Images::NestedComponent.new(f) %> diff --git a/app/views/legislation/proposals/_form.html.erb b/app/views/legislation/proposals/_form.html.erb index 69a699bf5..b1cf5d8b4 100644 --- a/app/views/legislation/proposals/_form.html.erb +++ b/app/views/legislation/proposals/_form.html.erb @@ -27,13 +27,13 @@
<% if feature?(:allow_images) %> -
- <%= render "images/nested_image", f: f %> +
+ <%= render Images::NestedComponent.new(f) %>
<% end %> -
- <%= render "documents/nested_documents", f: f %> +
+ <%= render Documents::NestedComponent.new(f) %>
<% if Geozone.any? %> From a8bd5eb19289552e8502e70bd7154a6752e7c209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 12 Oct 2023 21:38:08 +0200 Subject: [PATCH 09/14] Rename document/image fields HTML classes Using the `document` or `documents` classes meant styles defined for the public list of documents conflict with these ones. So now we're using HTML classes that match the name of the Ruby component classes, as we usually do. --- app/assets/javascripts/documentable.js | 6 ++++-- app/assets/javascripts/imageable.js | 2 +- .../stylesheets/admin/budget_phases/form.scss | 2 +- app/assets/stylesheets/mixins/uploads.scss | 4 ++-- .../attachable/fields_component.html.erb | 2 +- app/components/documents/nested_component.html.erb | 2 +- app/components/images/nested_component.html.erb | 2 +- spec/shared/system/nested_documentable.rb | 10 +++++----- spec/shared/system/nested_imageable.rb | 14 +++++++------- spec/support/common_actions/documents.rb | 4 ++-- spec/support/common_actions/images.rb | 4 ++-- 11 files changed, 27 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/documentable.js b/app/assets/javascripts/documentable.js index 3a2880da5..2a6829f5c 100644 --- a/app/assets/javascripts/documentable.js +++ b/app/assets/javascripts/documentable.js @@ -9,9 +9,11 @@ App.Documentable.unlockUploads(); }); $("#nested-documents").on("cocoon:after-insert", function(e, nested_document) { - var input; + var input, document_fields; input = $(nested_document).find(".js-document-attachment"); - input.lockUpload = $(nested_document).closest("#nested-documents").find(".document:visible").length >= $("#nested-documents").data("max-documents-allowed"); + document_fields = $(nested_document).closest("#nested-documents").find(".document-fields:visible"); + + input.lockUpload = document_fields.length >= $("#nested-documents").data("max-documents-allowed"); App.Documentable.initializeDirectUploadInput(input); if (input.lockUpload) { App.Documentable.lockUploads(); diff --git a/app/assets/javascripts/imageable.js b/app/assets/javascripts/imageable.js index 84ad8ce8f..82189d081 100644 --- a/app/assets/javascripts/imageable.js +++ b/app/assets/javascripts/imageable.js @@ -9,7 +9,7 @@ $("#new_image_link").removeClass("hide"); }); $("#nested-image").on("cocoon:before-insert", function() { - $(".js-image-attachment").closest(".image").remove(); + $(".js-image-attachment").closest(".image-fields").remove(); }); $("#nested-image").on("cocoon:after-insert", function(e, nested_image) { var input; diff --git a/app/assets/stylesheets/admin/budget_phases/form.scss b/app/assets/stylesheets/admin/budget_phases/form.scss index 693077b38..4faa8ffd0 100644 --- a/app/assets/stylesheets/admin/budget_phases/form.scss +++ b/app/assets/stylesheets/admin/budget_phases/form.scss @@ -23,7 +23,7 @@ } } - .images { + .images-nested { @include direct-uploads; } diff --git a/app/assets/stylesheets/mixins/uploads.scss b/app/assets/stylesheets/mixins/uploads.scss index 04db8ad91..fa5fdbf70 100644 --- a/app/assets/stylesheets/mixins/uploads.scss +++ b/app/assets/stylesheets/mixins/uploads.scss @@ -10,8 +10,8 @@ margin-bottom: $line-height; } - .document, - .image { + .document-fields, + .image-fields { .document-attachment, .image-attachment { diff --git a/app/components/attachable/fields_component.html.erb b/app/components/attachable/fields_component.html.erb index e5e9854a6..d51997e53 100644 --- a/app/components/attachable/fields_component.html.erb +++ b/app/components/attachable/fields_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= f.hidden_field :id %> <%= f.hidden_field :user_id, value: current_user.id %> <%= f.hidden_field :cached_attachment %> diff --git a/app/components/documents/nested_component.html.erb b/app/components/documents/nested_component.html.erb index d47185ae8..8d3bc2c63 100644 --- a/app/components/documents/nested_component.html.erb +++ b/app/components/documents/nested_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= t("documents.form.title") %>

<%= note %>

diff --git a/app/components/images/nested_component.html.erb b/app/components/images/nested_component.html.erb index 578cb53fe..58889a02c 100644 --- a/app/components/images/nested_component.html.erb +++ b/app/components/images/nested_component.html.erb @@ -1,4 +1,4 @@ -
+
<%= t("images.form.title") %>

<%= note %>

diff --git a/spec/shared/system/nested_documentable.rb b/spec/shared/system/nested_documentable.rb index d21c380ef..f73ab60e6 100644 --- a/spec/shared/system/nested_documentable.rb +++ b/spec/shared/system/nested_documentable.rb @@ -163,7 +163,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na click_link "Add new document" click_on submit_button - within "#nested-documents .document" do + within "#nested-documents .document-fields" do expect(page).to have_content("can't be blank", count: 2) end end @@ -175,7 +175,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na documentable_attach_new_file(file_fixture("empty.pdf")) click_link "Remove document" - expect(page).not_to have_css("#nested-documents .document") + expect(page).not_to have_css("#nested-documents .document-fields") end scenario "Should show successful notice when @@ -248,7 +248,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na do_login_for user_to_login, management: management visit send(path, arguments) - expect(page).to have_css ".document", count: 1 + expect(page).to have_css ".document-fields", count: 1 end scenario "Should not show add document button when @@ -264,7 +264,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na create_list(:document, documentable.class.max_documents_allowed, documentable: documentable) do_login_for user_to_login, management: management visit send(path, arguments) - last_document = all("#nested-documents .document").last + last_document = all("#nested-documents .document-fields").last within last_document do click_on "Remove document" end @@ -278,7 +278,7 @@ shared_examples "nested documentable" do |login_as_name, documentable_factory_na visit send(path, arguments) click_on "Remove document" - expect(page).not_to have_css ".document" + expect(page).not_to have_css ".document-fields" end scenario "Same attachment URL after editing the title" do diff --git a/spec/shared/system/nested_imageable.rb b/spec/shared/system/nested_imageable.rb index ca49a42d7..f055e055c 100644 --- a/spec/shared/system/nested_imageable.rb +++ b/spec/shared/system/nested_imageable.rb @@ -57,7 +57,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) click_link "Add image" - input_title = find(".image input[name$='[title]']") + input_title = find(".image-fields input[name$='[title]']") fill_in input_title[:id], with: "Title" attach_file "Choose image", file_fixture("clippy.jpg") @@ -119,7 +119,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p click_link "Add image" click_on submit_button - within "#nested-image .image" do + within "#nested-image .image-fields" do expect(page).to have_content("can't be blank", count: 2) end end @@ -143,11 +143,11 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p imageable_attach_new_file(file_fixture("clippy.jpg")) - within "#nested-image .image" do + within "#nested-image .image-fields" do click_link "Remove image" end - expect(page).not_to have_selector("#nested-image .image") + expect(page).not_to have_selector("#nested-image .image-fields") end scenario "Should show successful notice when resource filled correctly without any nested images", @@ -216,7 +216,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) - expect(page).to have_css ".image", count: 1 + expect(page).to have_css ".image-fields", count: 1 expect(page).not_to have_css "a#new_image_link" end @@ -227,7 +227,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p visit send(path, arguments) click_link "Remove image" - expect(page).not_to have_css ".image" + expect(page).not_to have_css ".image-fields" expect(page).to have_link id: "new_image_link" end @@ -239,7 +239,7 @@ shared_examples "nested imageable" do |imageable_factory_name, path, imageable_p click_link "Remove image" click_link "Add image" - expect(page).to have_css ".image", count: 1, visible: :all + expect(page).to have_css ".image-fields", count: 1, visible: :all end end end diff --git a/spec/support/common_actions/documents.rb b/spec/support/common_actions/documents.rb index c6b8e0b44..e7752ad84 100644 --- a/spec/support/common_actions/documents.rb +++ b/spec/support/common_actions/documents.rb @@ -9,7 +9,7 @@ module Documents def documentable_attach_new_file(path, success = true) click_link "Add new document" - document = all(".document").last + document = all(".document-fields").last attach_file "Choose document", path within document do @@ -22,7 +22,7 @@ module Documents end def expect_document_has_title(index, title) - document = all(".document")[index] + document = all(".document-fields")[index] within document do expect(find("input[name$='[title]']").value).to eq title diff --git a/spec/support/common_actions/images.rb b/spec/support/common_actions/images.rb index 8f372be43..1c7a248e1 100644 --- a/spec/support/common_actions/images.rb +++ b/spec/support/common_actions/images.rb @@ -11,7 +11,7 @@ module Images def imageable_attach_new_file(path, success = true) click_link "Add image" within "#nested-image" do - image = find(".image") + image = find(".image-fields") attach_file "Choose image", path within image do if success @@ -40,7 +40,7 @@ module Images end def expect_image_has_title(title) - image = find(".image") + image = find(".image-fields") within image do expect(find("input[name$='[title]']").value).to eq title From 5f0a7f4bbc42b82c7afb7450db5fa2fb2f181571 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Oct 2023 00:23:46 +0200 Subject: [PATCH 10/14] Group show document tests together While in unit tests it's great to have different tests for different expectations, system tests are slow, and so it's better to have just one test for all the expectations related to the same actions. --- spec/shared/system/documentable.rb | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/spec/shared/system/documentable.rb b/spec/shared/system/documentable.rb index 296da2bda..83b2624b7 100644 --- a/spec/shared/system/documentable.rb +++ b/spec/shared/system/documentable.rb @@ -11,22 +11,12 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, end context "Show documents" do - scenario "Download action should be able to anyone" do + 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") - end - - scenario "Download file link should have blank target attribute" do - visit send(documentable_path, arguments) - - expect(page).to have_selector("a[target=_blank]", text: "Download file") - end - - scenario "Download file links should have rel attribute setted to no follow" do - visit send(documentable_path, arguments) - - expect(page).to have_selector("a[rel=nofollow]", text: "Download file") + 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" end describe "Destroy action" do From e0c5be10aae81b60984277d68cf342f97f90cedc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Oct 2023 11:40:16 +0200 Subject: [PATCH 11/14] Extract mixins to style separators This way they can be reused in other places. --- app/assets/stylesheets/mixins/separators.scss | 23 +++++++++++++++++++ .../stylesheets/moderation_actions.scss | 20 ++-------------- 2 files changed, 25 insertions(+), 18 deletions(-) create mode 100644 app/assets/stylesheets/mixins/separators.scss diff --git a/app/assets/stylesheets/mixins/separators.scss b/app/assets/stylesheets/mixins/separators.scss new file mode 100644 index 000000000..f5bdfc606 --- /dev/null +++ b/app/assets/stylesheets/mixins/separators.scss @@ -0,0 +1,23 @@ +@mixin separator { + content: ""; + display: inline-block; + margin: 0 0.3em; + vertical-align: middle; +} + +@mixin vertical-separator { + @include separator; + background: currentcolor; + height: 1em; + opacity: 0.5; + width: 1px; +} + +@mixin circle-separator { + @include separator; + background: $text-light; + border-radius: 100%; + height: 0.25em; + opacity: 1; + width: 0.25em; +} diff --git a/app/assets/stylesheets/moderation_actions.scss b/app/assets/stylesheets/moderation_actions.scss index f3a00d662..dc77c9144 100644 --- a/app/assets/stylesheets/moderation_actions.scss +++ b/app/assets/stylesheets/moderation_actions.scss @@ -8,29 +8,13 @@ @include link; } - @mixin separator { - content: ""; - display: inline-block; - margin: 0 0.3em; - vertical-align: middle; - } - > * + *::before { - @include separator; - background: currentcolor; - height: 1em; - opacity: 0.5; - width: 1px; + @include vertical-separator; } .comment & { > *::before { - @include separator; - background: $text-light; - border-radius: 100%; - opacity: 1; - height: 0.25em; - width: 0.25em; + @include circle-separator; } } } From a2e4b056eef24d9d32b4272acc4dc12ba9f97f7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Oct 2023 11:58:14 +0200 Subject: [PATCH 12/14] Move documents partials to components This way it'll be easier to change them. Note that there were two `.document-link` elements which aren't part of a `.documents` element. We're renaming the HTML class of the link in investments because it didn't contain links to download documents and are slightly duplicating the CSS in the poll answer documents in order to keep the `word-wrap` property. --- app/assets/stylesheets/application.scss | 1 + .../stylesheets/documents/documents.scss | 37 ++++++++++++++++++ app/assets/stylesheets/layout.scss | 38 ------------------- app/assets/stylesheets/participation.scss | 8 ++++ .../documents/document_component.html.erb} | 0 .../documents/document_component.rb | 8 ++++ .../documents/documents_component.html.erb | 9 +++++ .../documents/documents_component.rb | 11 ++++++ .../investments/_investment_detail.html.erb | 4 +- .../dashboard/actions/new_request.html.erb | 2 +- app/views/documents/_documents.html.erb | 9 ----- app/views/legislation/proposals/show.html.erb | 2 +- app/views/proposals/_info.html.erb | 2 +- 13 files changed, 79 insertions(+), 52 deletions(-) create mode 100644 app/assets/stylesheets/documents/documents.scss rename app/{views/documents/_document.html.erb => components/documents/document_component.html.erb} (100%) create mode 100644 app/components/documents/document_component.rb create mode 100644 app/components/documents/documents_component.html.erb create mode 100644 app/components/documents/documents_component.rb delete mode 100644 app/views/documents/_documents.html.erb diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 068a33797..0d44d672e 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -47,6 +47,7 @@ @import "admin/**/*"; @import "budgets/**/*"; @import "debates/**/*"; +@import "documents/**/*"; @import "layout/**/*"; @import "machine_learning/**/*"; @import "moderation/**/*"; diff --git a/app/assets/stylesheets/documents/documents.scss b/app/assets/stylesheets/documents/documents.scss new file mode 100644 index 000000000..d4cf19acb --- /dev/null +++ b/app/assets/stylesheets/documents/documents.scss @@ -0,0 +1,37 @@ +.documents { + + h2 { + font-size: rem-calc(24); + + span { + color: #4f4f4f; + font-weight: normal; + } + } + + ul li { + padding-top: $line-height / 2; + + &:not(:first-child) { + border-top: 1px solid $highlight; + } + } + + .document-link { + background: $highlight-soft; + border: 2px solid $highlight; + border-radius: rem-calc(5); + display: block; + margin: $line-height / 2 0; + padding: $line-height / 2; + position: relative; + + p { + margin-bottom: 0; + } + + a { + word-wrap: break-word; + } + } +} diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 2710af584..b8ac99ee4 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -2016,44 +2016,6 @@ table { // 20. Documents // ------------- -.documents { - - h2 { - font-size: rem-calc(24); - - span { - color: #4f4f4f; - font-weight: normal; - } - } - - ul li { - padding-top: $line-height / 2; - - &:not(:first-child) { - border-top: 1px solid $highlight; - } - } - - .document-link { - background: $highlight-soft; - border: 2px solid $highlight; - border-radius: rem-calc(5); - display: block; - margin: $line-height / 2 0; - padding: $line-height / 2; - position: relative; - - p { - margin-bottom: 0; - } - } -} - -.document-link a { - word-wrap: break-word; -} - .additional-document-link { background: $highlight-soft; border: 1px solid $highlight; diff --git a/app/assets/stylesheets/participation.scss b/app/assets/stylesheets/participation.scss index 21a68e186..cca148e73 100644 --- a/app/assets/stylesheets/participation.scss +++ b/app/assets/stylesheets/participation.scss @@ -699,6 +699,10 @@ margin-top: rem-calc(10); } } + + .investment-external-link a { + word-wrap: break-word; + } } .budget-investment-show .supports { @@ -1437,6 +1441,10 @@ overflow: hidden; } } + + .document-link a { + word-wrap: break-word; + } } .orbit-bullets button { diff --git a/app/views/documents/_document.html.erb b/app/components/documents/document_component.html.erb similarity index 100% rename from app/views/documents/_document.html.erb rename to app/components/documents/document_component.html.erb diff --git a/app/components/documents/document_component.rb b/app/components/documents/document_component.rb new file mode 100644 index 000000000..ed418eda3 --- /dev/null +++ b/app/components/documents/document_component.rb @@ -0,0 +1,8 @@ +class Documents::DocumentComponent < ApplicationComponent + attr_reader :document + delegate :can?, to: :helpers + + def initialize(document) + @document = document + end +end diff --git a/app/components/documents/documents_component.html.erb b/app/components/documents/documents_component.html.erb new file mode 100644 index 000000000..d8912bd8c --- /dev/null +++ b/app/components/documents/documents_component.html.erb @@ -0,0 +1,9 @@ +
+

<%= t("documents.title") %> (<%= documents.count %>)

+ +
    + <% documents.each do |document| %> + <%= render Documents::DocumentComponent.new(document) %> + <% end %> +
+
diff --git a/app/components/documents/documents_component.rb b/app/components/documents/documents_component.rb new file mode 100644 index 000000000..5dfae9ecf --- /dev/null +++ b/app/components/documents/documents_component.rb @@ -0,0 +1,11 @@ +class Documents::DocumentsComponent < ApplicationComponent + attr_reader :documents + + def initialize(documents) + @documents = documents + end + + def render? + documents.any? + end +end diff --git a/app/views/budgets/investments/_investment_detail.html.erb b/app/views/budgets/investments/_investment_detail.html.erb index cab44899d..d8c8fff8f 100644 --- a/app/views/budgets/investments/_investment_detail.html.erb +++ b/app/views/budgets/investments/_investment_detail.html.erb @@ -45,13 +45,13 @@ <% end %> <% if feature?(:allow_attached_documents) %> - <%= render "documents/documents", documents: investment.documents %> + <%= render Documents::DocumentsComponent.new(investment.documents) %> <% end %> <%= render "shared/tags", taggable: investment %> <% if investment.external_url.present? %> -
+
<%= sanitize_and_auto_link investment.external_url %>
<% end %> diff --git a/app/views/dashboard/actions/new_request.html.erb b/app/views/dashboard/actions/new_request.html.erb index 4359f17c8..93549ec36 100644 --- a/app/views/dashboard/actions/new_request.html.erb +++ b/app/views/dashboard/actions/new_request.html.erb @@ -7,7 +7,7 @@
- <%= render "documents/documents", documents: dashboard_action.documents %> + <%= render Documents::DocumentsComponent.new(dashboard_action.documents) %> <% if dashboard_action.links.any? %>
diff --git a/app/views/documents/_documents.html.erb b/app/views/documents/_documents.html.erb deleted file mode 100644 index a828ccc90..000000000 --- a/app/views/documents/_documents.html.erb +++ /dev/null @@ -1,9 +0,0 @@ -<% if documents.any? %> -
-

<%= t("documents.title") %> (<%= documents.count %>)

- -
    - <%= render partial: "documents/document", collection: documents %> -
-
-<% end %> diff --git a/app/views/legislation/proposals/show.html.erb b/app/views/legislation/proposals/show.html.erb index ec31d0f88..7f9123bd4 100644 --- a/app/views/legislation/proposals/show.html.erb +++ b/app/views/legislation/proposals/show.html.erb @@ -87,7 +87,7 @@ <% end %> <% if feature?(:allow_attached_documents) %> - <%= render "documents/documents", documents: @proposal.documents %> + <%= render Documents::DocumentsComponent.new(@proposal.documents) %> <% end %> <%= render "shared/tags", taggable: @proposal %> diff --git a/app/views/proposals/_info.html.erb b/app/views/proposals/_info.html.erb index 6bb723136..23def8f68 100644 --- a/app/views/proposals/_info.html.erb +++ b/app/views/proposals/_info.html.erb @@ -67,7 +67,7 @@ <% end %> <% if feature?(:allow_attached_documents) %> - <%= render "documents/documents", documents: @proposal.documents %> + <%= render Documents::DocumentsComponent.new(@proposal.documents) %> <% end %> <%= render "shared/tags", taggable: @proposal %> 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 13/14] 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 @@
      <% documents.each do |document| %> - <%= render Documents::DocumentComponent.new(document) %> +
    • + <%= render Documents::DocumentComponent.new(document, show_destroy_link: true) %> +
      • <% end %>
    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 From cdc5e05d480c12365f507c04b1930e0a54fb4a52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 13 Oct 2023 13:02:19 +0200 Subject: [PATCH 14/14] Open PDF files in the same tab/window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Quoting usability experts Jakob Nielsen and Anna Kaley [1]: > [Opening PDF files in new tabs] is problematic, because it assumes > users will always do the exact same things with certain file formats, > which isn’t always the case. There are many examples of this situation. For example, some people (myself included) configure their browser so it downloads PDF files instead of opening them in the browser. In this situation, a new tab is opened, a blank page is displayed, the file is downloaded, and then either the tab is closed or the blank page needs to be manually closed. The end result is really annoying. Other situations include people who use a mobile phone browser, where navigating through tabs is generally much harder than doing so on a desktop browser. But IMHO the most important point is: every browser already provides a way to open "regular" links in a new tab, so people can choose what to do, but if we decide to open the link in a new tab, we take control away from them, and people who'd like to open the link in the same tab might feel frustrated. In these cases, the links either say "download" or include the word "PDF", so people know in advance that they're going to download/open a PDF file, and so we're giving them information and, by removing the `target` attribute, we're giving them control over their browser so they can choose what's convenient for them. [1] https://www.nngroup.com/articles/new-browser-windows-and-tabs --- .../answers/documents/table_actions_component.html.erb | 1 - app/components/documents/document_component.html.erb | 2 +- app/views/admin/milestones/_milestones.html.erb | 1 - .../admin/site_customization/documents/index.html.erb | 1 - app/views/dashboard/poster/_poster_options.html.erb | 1 - spec/shared/system/documentable.rb | 4 ++-- spec/system/dashboard/poster_spec.rb | 8 ++++---- 7 files changed, 7 insertions(+), 11 deletions(-) diff --git a/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb b/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb index 19e8e3ac2..37f978cdf 100644 --- a/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb +++ b/app/components/admin/poll/questions/answers/documents/table_actions_component.html.erb @@ -3,6 +3,5 @@ <%= actions.action(:download, text: t("documents.buttons.download_document"), path: document.attachment, - target: "_blank", rel: "nofollow") %> <% end %> diff --git a/app/components/documents/document_component.html.erb b/app/components/documents/document_component.html.erb index 2709de4b1..1de910081 100644 --- a/app/components/documents/document_component.html.erb +++ b/app/components/documents/document_component.html.erb @@ -1,5 +1,5 @@
    - <%= link_to document.attachment, target: "_blank", rel: "nofollow" do %> + <%= link_to document.attachment, rel: "nofollow" do %> <%= document.title %> <%= document.humanized_content_type %> diff --git a/app/views/admin/milestones/_milestones.html.erb b/app/views/admin/milestones/_milestones.html.erb index 46940ab88..2a02ea6d8 100644 --- a/app/views/admin/milestones/_milestones.html.erb +++ b/app/views/admin/milestones/_milestones.html.erb @@ -48,7 +48,6 @@ <% milestone.documents.each do |document| %> <%= link_to document.title, document.attachment, - target: "_blank", rel: "nofollow" %>
    <% 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 85dd876e0..b3f4ca635 100644 --- a/app/views/admin/site_customization/documents/index.html.erb +++ b/app/views/admin/site_customization/documents/index.html.erb @@ -31,7 +31,6 @@ <%= actions.action(:download, text: t("documents.buttons.download_document"), path: document.attachment, - target: "_blank", rel: "nofollow") %> <% end %> diff --git a/app/views/dashboard/poster/_poster_options.html.erb b/app/views/dashboard/poster/_poster_options.html.erb index 27e1cc1eb..f807d5954 100644 --- a/app/views/dashboard/poster/_poster_options.html.erb +++ b/app/views/dashboard/poster/_poster_options.html.erb @@ -7,6 +7,5 @@ <%= link_to t("dashboard.poster.options.download"), proposal_dashboard_poster_index_path(proposal, format: :pdf), - target: "_blank", class: "button expanded" %>
    diff --git a/spec/shared/system/documentable.rb b/spec/shared/system/documentable.rb index d4eda590e..6723c55a2 100644 --- a/spec/shared/system/documentable.rb +++ b/spec/shared/system/documentable.rb @@ -11,13 +11,13 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, end context "Show documents" do - scenario "Download action should be availabe to anyone and open in a new window" do + scenario "Download action should be availabe to anyone and open in the same tab" do visit send(documentable_path, arguments) 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 + expect(page).not_to have_selector "a[target=_blank]" end end diff --git a/spec/system/dashboard/poster_spec.rb b/spec/system/dashboard/poster_spec.rb index ad591a905..bf311d570 100644 --- a/spec/system/dashboard/poster_spec.rb +++ b/spec/system/dashboard/poster_spec.rb @@ -31,9 +31,9 @@ describe "Poster" do end scenario "PDF contains the proposal details" do - within_window(window_opened_by { click_link "Download" }) do - expect(page).to have_content(proposal.title) - expect(page).to have_content(proposal.code) - end + click_link "Download" + + expect(page).to have_content(proposal.title) + expect(page).to have_content(proposal.code) end end