From 2fb8abe83fdaf0d442999e3ef1580b5d2437563d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 10 Oct 2024 23:16:09 +0200 Subject: [PATCH 01/14] Use a button to delete documents While testing for accessibility issues (in a development branch), we're removing Turbolinks and monkey-patching the behavior of the `click_link` method to check the page for accessibility issues after each request. However, we were getting false positives when clicking links that act like buttons. So, for the reasons mentioned in commit 5311daadf, we're replacing the link to delete a document with a button. --- app/assets/stylesheets/documents/document.scss | 6 +++++- .../documents/document_component.html.erb | 10 +++++----- spec/shared/system/documentable.rb | 14 +++++++------- 3 files changed, 17 insertions(+), 13 deletions(-) diff --git a/app/assets/stylesheets/documents/document.scss b/app/assets/stylesheets/documents/document.scss index 54ecd651c..2b217757d 100644 --- a/app/assets/stylesheets/documents/document.scss +++ b/app/assets/stylesheets/documents/document.scss @@ -3,7 +3,7 @@ margin-top: calc(#{$line-height} / 3); } - a:first-of-type { + a { word-wrap: break-word; .document-metadata { @@ -18,4 +18,8 @@ } } } + + button { + cursor: pointer; + } } diff --git a/app/components/documents/document_component.html.erb b/app/components/documents/document_component.html.erb index 1de910081..f31ba2a12 100644 --- a/app/components/documents/document_component.html.erb +++ b/app/components/documents/document_component.html.erb @@ -8,10 +8,10 @@ <% end %> <% 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" %> + <%= button_to t("documents.buttons.destroy_document"), + document, + method: :delete, + data: { confirm: t("documents.actions.destroy.confirm") }, + class: "delete" %> <% end %> diff --git a/spec/shared/system/documentable.rb b/spec/shared/system/documentable.rb index ad6727671..3f7177573 100644 --- a/spec/shared/system/documentable.rb +++ b/spec/shared/system/documentable.rb @@ -25,27 +25,27 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, scenario "Should not be able when no user logged in" do visit send(documentable_path, arguments) - expect(page).not_to have_link("Delete document") + expect(page).not_to have_button "Delete document" end scenario "Should be able when documentable author is logged in" do login_as documentable.author visit send(documentable_path, arguments) - expect(page).to have_link("Delete document") + expect(page).to have_button "Delete document" end scenario "Administrators cannot destroy documentables they have not authored", :admin do visit send(documentable_path, arguments) - expect(page).not_to have_link("Delete document") + expect(page).not_to have_button "Delete document" end scenario "Users cannot destroy documentables they have not authored" do login_as(create(:user)) visit send(documentable_path, arguments) - expect(page).not_to have_link("Delete document") + expect(page).not_to have_button "Delete document" end end @@ -93,7 +93,7 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, visit send(documentable_path, arguments) within "#document_#{document.id}" do - accept_confirm { click_link "Delete document" } + accept_confirm { click_button "Delete document" } end expect(page).to have_content "Document was deleted successfully." @@ -105,7 +105,7 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, visit send(documentable_path, arguments) within "#document_#{document.id}" do - accept_confirm { click_link "Delete document" } + accept_confirm { click_button "Delete document" } end expect(page).not_to have_content "Documents (0)" @@ -117,7 +117,7 @@ shared_examples "documentable" do |documentable_factory_name, documentable_path, visit send(documentable_path, arguments) within "#document_#{document.id}" do - accept_confirm { click_link "Delete document" } + accept_confirm { click_button "Delete document" } end within "##{ActionView::RecordIdentifier.dom_id(documentable)}" do From 58cba2316aedb3efefe32a6b41c10fdce7d78f7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 03:13:56 +0200 Subject: [PATCH 02/14] Use a button to erase an account in the management area As mentioned in commit 5311daadf, there are several reasons to use buttons in these situations. And, as mentioned in the previous commit, using buttons instead of links for actions requiring confirmation will help us test for accessibility issues. --- app/views/management/users/_erase_user_account.html.erb | 2 +- spec/system/management/users_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/management/users/_erase_user_account.html.erb b/app/views/management/users/_erase_user_account.html.erb index 5746e103a..f7089203a 100644 --- a/app/views/management/users/_erase_user_account.html.erb +++ b/app/views/management/users/_erase_user_account.html.erb @@ -5,5 +5,5 @@ <%= t("management.users.erase_warning") %> - <%= link_to t("management.users.erase_submit"), erase_management_users_path, method: :delete, class: "button hollow alert", data: { confirm: t("management.users.erase_account_confirm") } %> + <%= button_to t("management.users.erase_submit"), erase_management_users_path, method: :delete, class: "button hollow alert", data: { confirm: t("management.users.erase_account_confirm") } %> diff --git a/spec/system/management/users_spec.rb b/spec/system/management/users_spec.rb index 2291e9f56..fccf14b29 100644 --- a/spec/system/management/users_spec.rb +++ b/spec/system/management/users_spec.rb @@ -82,7 +82,7 @@ describe "Users" do expect(page).to have_content "This user can participate in the website with the following permissions" click_link "Delete user" - accept_confirm { click_link "Delete account" } + accept_confirm { click_button "Delete account" } expect(page).to have_content "User account deleted." From 0e2434c0945082f797a10410655e7e51366a6493 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 16:15:34 +0200 Subject: [PATCH 03/14] Extract commponent to render user investment actions This way it'll be easier to organize code related to it. --- .../budget_investment_table_actions_component.html.erb | 9 +++++++++ .../users/budget_investment_table_actions_component.rb | 8 ++++++++ app/views/users/_budget_investment.html.erb | 10 +--------- 3 files changed, 18 insertions(+), 9 deletions(-) create mode 100644 app/components/users/budget_investment_table_actions_component.html.erb create mode 100644 app/components/users/budget_investment_table_actions_component.rb diff --git a/app/components/users/budget_investment_table_actions_component.html.erb b/app/components/users/budget_investment_table_actions_component.html.erb new file mode 100644 index 000000000..45674e2b2 --- /dev/null +++ b/app/components/users/budget_investment_table_actions_component.html.erb @@ -0,0 +1,9 @@ +<% if can? :update, investment %> + <%= link_to t("shared.edit"), edit_budget_investment_path(investment.budget, investment), + class: "button hollow" %> +<% end %> +<% if can? :destroy, investment %> + <%= link_to t("shared.delete"), budget_investment_path(investment.budget, investment), + method: :delete, class: "button hollow alert", + data: { confirm: t("users.show.delete_alert") } %> +<% end %> diff --git a/app/components/users/budget_investment_table_actions_component.rb b/app/components/users/budget_investment_table_actions_component.rb new file mode 100644 index 000000000..9628a4383 --- /dev/null +++ b/app/components/users/budget_investment_table_actions_component.rb @@ -0,0 +1,8 @@ +class Users::BudgetInvestmentTableActionsComponent < ApplicationComponent + attr_reader :investment + use_helpers :can? + + def initialize(investment) + @investment = investment + end +end diff --git a/app/views/users/_budget_investment.html.erb b/app/views/users/_budget_investment.html.erb index 7c9dc0fec..81377519c 100644 --- a/app/views/users/_budget_investment.html.erb +++ b/app/views/users/_budget_investment.html.erb @@ -3,14 +3,6 @@ <%= link_to budget_investment.title, budget_investment_path(budget_investment.budget, budget_investment) %> - <% if can? :update, budget_investment %> - <%= link_to t("shared.edit"), edit_budget_investment_path(budget_investment.budget, budget_investment), - class: "button hollow" %> - <% end %> - <% if can? :destroy, budget_investment %> - <%= link_to t("shared.delete"), budget_investment_path(budget_investment.budget, budget_investment), - method: :delete, class: "button hollow alert", - data: { confirm: t("users.show.delete_alert") } %> - <% end %> + <%= render Users::BudgetInvestmentTableActionsComponent.new(budget_investment) %> From b694ee70771a74053b96ce33f49f7c3f74890489 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 03:54:00 +0200 Subject: [PATCH 04/14] Use a button to delete an investment Note that, since the button now generates a `form` tag, we need to adjust the styles of this section. As mentioned in commit 5311daadf, there are several reasons to use buttons in these situations. And, as mentioned in the previous commits, using buttons instead of links for actions requiring confirmation will help us test for accessibility issues. Note we're simplifying the `table .button` margin rules because the `.button` class already defines `0` for all its margins except the bottom margin. Otherwise, the margins defined by the `flex-with-gap` mixin would be overwritten by the margins defined in the `table .button` class. --- app/assets/stylesheets/application.scss | 1 + app/assets/stylesheets/layout.scss | 2 +- .../budget_investment_table_actions.scss | 3 +++ ...nvestment_table_actions_component.html.erb | 20 ++++++++++--------- spec/system/budgets/investments_spec.rb | 2 +- spec/system/users_spec.rb | 4 ++-- 6 files changed, 19 insertions(+), 13 deletions(-) create mode 100644 app/assets/stylesheets/users/budget_investment_table_actions.scss diff --git a/app/assets/stylesheets/application.scss b/app/assets/stylesheets/application.scss index 1da88b08a..b2badb487 100644 --- a/app/assets/stylesheets/application.scss +++ b/app/assets/stylesheets/application.scss @@ -56,6 +56,7 @@ @import "sdg_management/**/*"; @import "shared/**/*"; @import "subscriptions"; +@import "users/**/*"; @import "widgets/**/*"; @import "custom"; diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 87a2f08ec..54e808c50 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1245,7 +1245,7 @@ table { } .button { - margin: 0; + margin-bottom: 0; } } diff --git a/app/assets/stylesheets/users/budget_investment_table_actions.scss b/app/assets/stylesheets/users/budget_investment_table_actions.scss new file mode 100644 index 000000000..37e128101 --- /dev/null +++ b/app/assets/stylesheets/users/budget_investment_table_actions.scss @@ -0,0 +1,3 @@ +.user-budget-investment-table-actions { + @include flex-with-gap($line-height * 0.25); +} diff --git a/app/components/users/budget_investment_table_actions_component.html.erb b/app/components/users/budget_investment_table_actions_component.html.erb index 45674e2b2..ea9290bcd 100644 --- a/app/components/users/budget_investment_table_actions_component.html.erb +++ b/app/components/users/budget_investment_table_actions_component.html.erb @@ -1,9 +1,11 @@ -<% if can? :update, investment %> - <%= link_to t("shared.edit"), edit_budget_investment_path(investment.budget, investment), - class: "button hollow" %> -<% end %> -<% if can? :destroy, investment %> - <%= link_to t("shared.delete"), budget_investment_path(investment.budget, investment), - method: :delete, class: "button hollow alert", - data: { confirm: t("users.show.delete_alert") } %> -<% end %> +
+ <% if can? :update, investment %> + <%= link_to t("shared.edit"), edit_budget_investment_path(investment.budget, investment), + class: "button hollow" %> + <% end %> + <% if can? :destroy, investment %> + <%= button_to t("shared.delete"), budget_investment_path(investment.budget, investment), + method: :delete, class: "button hollow alert", + data: { confirm: t("users.show.delete_alert") } %> + <% end %> +
diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb index 31ac65b28..f9a08fdda 100644 --- a/spec/system/budgets/investments_spec.rb +++ b/spec/system/budgets/investments_spec.rb @@ -1170,7 +1170,7 @@ describe "Budget Investments" do within("#budget_investment_#{investment1.id}") do expect(page).to have_content(investment1.title) - accept_confirm { click_link("Delete") } + accept_confirm { click_button "Delete" } end expect(page).to have_content "Investment project deleted successfully" diff --git a/spec/system/users_spec.rb b/spec/system/users_spec.rb index 34e3c19c4..c6ddd1d4e 100644 --- a/spec/system/users_spec.rb +++ b/spec/system/users_spec.rb @@ -131,12 +131,12 @@ describe "Users" do expect(page).to have_link budget_investment.title within("#budget_investment_#{budget_investment.id}") do - dismiss_confirm { click_link "Delete" } + dismiss_confirm { click_button "Delete" } end expect(page).to have_link budget_investment.title within("#budget_investment_#{budget_investment.id}") do - accept_confirm { click_link "Delete" } + accept_confirm { click_button "Delete" } end expect(page).not_to have_link budget_investment.title end From cbdf2f7f220d27653eaf4bf578f63f66753e01ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 16:44:25 +0200 Subject: [PATCH 05/14] Extract methods in user investment table actions --- ...t_investment_table_actions_component.html.erb | 8 +++----- .../budget_investment_table_actions_component.rb | 16 ++++++++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/app/components/users/budget_investment_table_actions_component.html.erb b/app/components/users/budget_investment_table_actions_component.html.erb index ea9290bcd..a29abf847 100644 --- a/app/components/users/budget_investment_table_actions_component.html.erb +++ b/app/components/users/budget_investment_table_actions_component.html.erb @@ -1,11 +1,9 @@
<% if can? :update, investment %> - <%= link_to t("shared.edit"), edit_budget_investment_path(investment.budget, investment), - class: "button hollow" %> + <%= edit_link %> <% end %> + <% if can? :destroy, investment %> - <%= button_to t("shared.delete"), budget_investment_path(investment.budget, investment), - method: :delete, class: "button hollow alert", - data: { confirm: t("users.show.delete_alert") } %> + <%= destroy_button %> <% end %>
diff --git a/app/components/users/budget_investment_table_actions_component.rb b/app/components/users/budget_investment_table_actions_component.rb index 9628a4383..8a12a4e33 100644 --- a/app/components/users/budget_investment_table_actions_component.rb +++ b/app/components/users/budget_investment_table_actions_component.rb @@ -5,4 +5,20 @@ class Users::BudgetInvestmentTableActionsComponent < ApplicationComponent def initialize(investment) @investment = investment end + + private + + def edit_link + link_to t("shared.edit"), + edit_budget_investment_path(investment.budget, investment), + class: "button hollow" + end + + def destroy_button + button_to t("shared.delete"), + budget_investment_path(investment.budget, investment), + method: :delete, + class: "button hollow alert", + data: { confirm: t("users.show.delete_alert") } + end end From 2fb8eaf6c7e9a2b90cd643320d6ad3bbfccc75eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 17:03:45 +0200 Subject: [PATCH 06/14] Add aria-labels to user investment actions This way it'll be easier for people using screen readers to know which link/button they're about to click. Note that, at least for now, we aren't reusing the code en `Admin::ActionComponent`. We might do so in the future if we implement similar code in more parts of the public area. --- ...dget_investment_table_actions_component.rb | 6 ++++ config/locales/en/general.yml | 2 ++ config/locales/es/general.yml | 2 ++ ...investment_table_actions_component_spec.rb | 31 +++++++++++++++++++ 4 files changed, 41 insertions(+) create mode 100644 spec/components/users/budget_investment_table_actions_component_spec.rb diff --git a/app/components/users/budget_investment_table_actions_component.rb b/app/components/users/budget_investment_table_actions_component.rb index 8a12a4e33..e3dfb12db 100644 --- a/app/components/users/budget_investment_table_actions_component.rb +++ b/app/components/users/budget_investment_table_actions_component.rb @@ -11,6 +11,7 @@ class Users::BudgetInvestmentTableActionsComponent < ApplicationComponent def edit_link link_to t("shared.edit"), edit_budget_investment_path(investment.budget, investment), + "aria-label": action_label(t("shared.edit")), class: "button hollow" end @@ -18,7 +19,12 @@ class Users::BudgetInvestmentTableActionsComponent < ApplicationComponent button_to t("shared.delete"), budget_investment_path(investment.budget, investment), method: :delete, + "aria-label": action_label(t("shared.delete")), class: "button hollow alert", data: { confirm: t("users.show.delete_alert") } end + + def action_label(action_text) + t("shared.actions.label", action: action_text, name: investment.title) + end end diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 5168cbd9d..95a9db662 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -644,6 +644,8 @@ en: show: back: "Go back to my content" shared: + actions: + label: "%{action} %{name}" edit: "Edit" filter: "Filter" save: "Save" diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index e8bf22da2..05071f11f 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -644,6 +644,8 @@ es: show: back: "Volver a mi contenido" shared: + actions: + label: "%{action} %{name}" edit: "Editar" filter: "Filtrar" save: "Guardar" diff --git a/spec/components/users/budget_investment_table_actions_component_spec.rb b/spec/components/users/budget_investment_table_actions_component_spec.rb new file mode 100644 index 000000000..400721f8a --- /dev/null +++ b/spec/components/users/budget_investment_table_actions_component_spec.rb @@ -0,0 +1,31 @@ +require "rails_helper" + +describe Users::BudgetInvestmentTableActionsComponent do + let(:user) { create(:user, :level_two) } + let(:budget) { create(:budget, :accepting) } + let(:investment) { create(:budget_investment, budget: budget, author: user, title: "User investment") } + + describe "#edit_link" do + it "generates an aria-label attribute" do + sign_in(user) + + render_inline Users::BudgetInvestmentTableActionsComponent.new(investment) + + expect(page).to have_link count: 1 + expect(page).to have_link "Edit" + expect(page).to have_css "a[aria-label='Edit User investment']" + end + end + + describe "#destroy_button" do + it "generates an aria-label attribute" do + sign_in(user) + + render_inline Users::BudgetInvestmentTableActionsComponent.new(investment) + + expect(page).to have_button count: 1 + expect(page).to have_button "Delete" + expect(page).to have_css "button[aria-label='Delete User investment']" + end + end +end From 891333abed793bb4dd29746a38af8a6b407899f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 04:21:02 +0200 Subject: [PATCH 07/14] Use a button to hide recommendations As mentioned in commit 5311daadf, there are several reasons to use buttons in these situations. And, as mentioned in the previous commits, using buttons instead of links for actions requiring confirmation will help us test for accessibility issues. Since we're adding styles for this button, we're also adding the `font-size` property instead of using the `small` class. We'll deal with the `float-right` property in the next commit. --- app/assets/stylesheets/layout.scss | 8 ++++++++ app/views/shared/_recommended_index.html.erb | 14 +++++++------- spec/system/debates_spec.rb | 2 +- spec/system/proposals_spec.rb | 2 +- 4 files changed, 17 insertions(+), 9 deletions(-) diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 54e808c50..f3c966934 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1943,10 +1943,18 @@ table { .hide-recommendations { color: $text-light; + cursor: pointer; + font-size: $small-font-size; + line-height: inherit; position: absolute; right: 12px; top: -18px; z-index: 2; + + &:focus, + &:hover { + @include anchor-color-hover; + } } // 20. Documents diff --git a/app/views/shared/_recommended_index.html.erb b/app/views/shared/_recommended_index.html.erb index 4447a9ae4..bf7bf13eb 100644 --- a/app/views/shared/_recommended_index.html.erb +++ b/app/views/shared/_recommended_index.html.erb @@ -5,13 +5,13 @@
- <%= link_to disable_recommendations_path, title: t("shared.recommended_index.hide"), - class: "float-right-medium small hide-recommendations", - data: { - toggle: "recommendations", - confirm: t("#{namespace}.index.recommendations.disable") - }, - method: :put do %> + <%= button_to disable_recommendations_path, title: t("shared.recommended_index.hide"), + class: "float-right-medium hide-recommendations", + data: { + toggle: "recommendations", + confirm: t("#{namespace}.index.recommendations.disable") + }, + method: :put do %> <%= t("shared.recommended_index.hide") %> <% end %> diff --git a/spec/system/debates_spec.rb b/spec/system/debates_spec.rb index 68d5de88c..ff1b499d5 100644 --- a/spec/system/debates_spec.rb +++ b/spec/system/debates_spec.rb @@ -521,7 +521,7 @@ describe "Debates" do expect(page).to have_content("Medium") expect(page).to have_css(".recommendation", count: 3) - accept_confirm { click_link "Hide recommendations" } + accept_confirm { click_button "Hide recommendations" } end expect(page).not_to have_link("recommendations") diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 459f7c619..4ea70abe0 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -937,7 +937,7 @@ describe "Proposals" do expect(page).to have_content("Medium") expect(page).to have_css(".recommendation", count: 3) - accept_confirm { click_link "Hide recommendations" } + accept_confirm { click_button "Hide recommendations" } end expect(page).not_to have_link("recommendations") From 26b24af413c8a0e052b74b11de2b0f5c0c6b5d19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2024 12:55:48 +0100 Subject: [PATCH 08/14] Remove unused HTML class in hide recommendation button Since the element uses `position: absolute`, the `float: right` property set by this utility class is ignored. --- app/views/shared/_recommended_index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/_recommended_index.html.erb b/app/views/shared/_recommended_index.html.erb index bf7bf13eb..eb3783d58 100644 --- a/app/views/shared/_recommended_index.html.erb +++ b/app/views/shared/_recommended_index.html.erb @@ -6,7 +6,7 @@
<%= button_to disable_recommendations_path, title: t("shared.recommended_index.hide"), - class: "float-right-medium hide-recommendations", + class: "hide-recommendations", data: { toggle: "recommendations", confirm: t("#{namespace}.index.recommendations.disable") From 11ef917802f7e4f393503c3153f6d35abcf8b627 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 04:24:20 +0200 Subject: [PATCH 09/14] Use a button to delete comments As mentioned in commit 5311daadf, there are several reasons to use buttons in these situations. And, as mentioned in the previous commits, using buttons instead of links for actions requiring confirmation will help us test for accessibility issues. --- app/assets/stylesheets/layout.scss | 5 +++++ app/views/comments/_actions.html.erb | 8 ++++---- spec/system/comments_spec.rb | 8 ++++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index f3c966934..192baa5c2 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1543,8 +1543,13 @@ table { } } + .delete-comment-form { + display: inline; + } + .delete-comment { @include has-fa-icon(trash-alt, regular); + cursor: pointer; &:not(:hover):not(:active) { color: $delete; diff --git a/app/views/comments/_actions.html.erb b/app/views/comments/_actions.html.erb index 7780d9167..17f5ee0c7 100644 --- a/app/views/comments/_actions.html.erb +++ b/app/views/comments/_actions.html.erb @@ -5,10 +5,10 @@ <% if can?(:hide, comment) || can?(:hide, comment.user) %> <% if comment.author == current_user %>  •  - <%= link_to t("comments.actions.delete"), - hide_comment_path(comment), - method: :put, remote: true, class: "delete-comment", - data: { confirm: t("comments.actions.confirm_delete") } %> + <%= button_to t("comments.actions.delete"), + hide_comment_path(comment), + method: :put, remote: true, class: "delete-comment", form_class: "delete-comment-form", + data: { confirm: t("comments.actions.confirm_delete") } %> <% else %> <%= render Shared::ModerationActionsComponent.new(comment) %> <% end %> diff --git a/spec/system/comments_spec.rb b/spec/system/comments_spec.rb index c8a33de68..0702c1b2c 100644 --- a/spec/system/comments_spec.rb +++ b/spec/system/comments_spec.rb @@ -336,16 +336,16 @@ describe "Comments" do visit polymorphic_path(resource) accept_confirm("Are you sure? This action will delete this comment. You can't undo this action.") do - within(".comment-body", text: "This was a mistake") { click_link "Delete comment" } + within(".comment-body", text: "This was a mistake") { click_button "Delete comment" } end expect(page).not_to have_content "This was a mistake" - expect(page).not_to have_link "Delete comment" + expect(page).not_to have_button "Delete comment" refresh expect(page).not_to have_content "This was a mistake" - expect(page).not_to have_link "Delete comment" + expect(page).not_to have_button "Delete comment" logout login_as(admin) @@ -363,7 +363,7 @@ describe "Comments" do visit polymorphic_path(resource) accept_confirm("Are you sure? This action will delete this comment. You can't undo this action.") do - within(".comment-body", text: "Wrong comment") { click_link "Delete comment" } + within(".comment-body", text: "Wrong comment") { click_button "Delete comment" } end within "#comments > .comment-list > li", text: "Right reply" do From d85a87a517e8584ee6c6183baaee56c7e8b2bf1d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 04:39:32 +0200 Subject: [PATCH 10/14] Use a button to delete surveys As mentioned in commit 5311daadf, there are several reasons to use buttons in these situations. And, as mentioned in the previous commits, using buttons instead of links for actions requiring confirmation will help us test for accessibility issues. --- app/assets/stylesheets/dashboard.scss | 8 ++++++++ app/views/dashboard/polls/_poll.html.erb | 10 +++++----- spec/system/dashboard/polls_spec.rb | 4 ++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/assets/stylesheets/dashboard.scss b/app/assets/stylesheets/dashboard.scss index a9755e38e..60bfe0fb0 100644 --- a/app/assets/stylesheets/dashboard.scss +++ b/app/assets/stylesheets/dashboard.scss @@ -442,6 +442,14 @@ .button { font-weight: bold; } + + button { + cursor: pointer; + + &:hover { + text-decoration: underline; + } + } } .community-poll { diff --git a/app/views/dashboard/polls/_poll.html.erb b/app/views/dashboard/polls/_poll.html.erb index 26fe74c28..4c3eca69d 100644 --- a/app/views/dashboard/polls/_poll.html.erb +++ b/app/views/dashboard/polls/_poll.html.erb @@ -27,10 +27,10 @@ <% end %>

<%= t("dashboard.polls.poll.show_results_help") %>

- <%= link_to t("dashboard.polls.poll.delete"), - proposal_dashboard_poll_path(proposal, poll), - method: :delete, - "data-confirm": t("dashboard.polls.poll.alert_notice"), - class: "delete" %> + <%= button_to t("dashboard.polls.poll.delete"), + proposal_dashboard_poll_path(proposal, poll), + method: :delete, + "data-confirm": t("dashboard.polls.poll.alert_notice"), + class: "delete" %>
diff --git a/spec/system/dashboard/polls_spec.rb b/spec/system/dashboard/polls_spec.rb index 0f7d3d61b..0620cdcf3 100644 --- a/spec/system/dashboard/polls_spec.rb +++ b/spec/system/dashboard/polls_spec.rb @@ -199,7 +199,7 @@ describe "Polls" do visit proposal_dashboard_polls_path(proposal) within("#poll_#{poll.id}") do - accept_confirm { click_link "Delete survey" } + accept_confirm { click_button "Delete survey" } end expect(page).to have_content("Survey deleted successfully") @@ -214,7 +214,7 @@ describe "Polls" do visit proposal_dashboard_polls_path(proposal) within("#poll_#{poll.id}") do - accept_confirm { click_link "Delete survey" } + accept_confirm { click_button "Delete survey" } end expect(page).to have_content("You cannot destroy a survey that has responses") From f8faabf7d1bedffbd32ba81cb4a0bf2eb483823f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 17:43:16 +0200 Subject: [PATCH 11/14] Extract component to mark a debate as featured We're also moving the path argument in the `link_to` calls to a different line, since it's what we usually do. --- .../mark_featured_action_component.html.erb | 16 ++++++++++++++++ .../debates/mark_featured_action_component.rb | 12 ++++++++++++ app/views/debates/_actions.html.erb | 18 +----------------- 3 files changed, 29 insertions(+), 17 deletions(-) create mode 100644 app/components/debates/mark_featured_action_component.html.erb create mode 100644 app/components/debates/mark_featured_action_component.rb diff --git a/app/components/debates/mark_featured_action_component.html.erb b/app/components/debates/mark_featured_action_component.html.erb new file mode 100644 index 000000000..3eb392fcf --- /dev/null +++ b/app/components/debates/mark_featured_action_component.html.erb @@ -0,0 +1,16 @@ + |  +<% if debate.featured? %> + <%= link_to t("admin.actions.unmark_featured").capitalize, + unmark_featured_debate_path(debate), + method: :put, + data: { confirm: t("admin.actions.confirm_action", + action: t("admin.actions.unmark_featured"), + name: debate.title) } %> +<% else %> + <%= link_to t("admin.actions.mark_featured").capitalize, + mark_featured_debate_path(debate), + method: :put, + data: { confirm: t("admin.actions.confirm_action", + action: t("admin.actions.mark_featured"), + name: debate.title) } %> +<% end %> diff --git a/app/components/debates/mark_featured_action_component.rb b/app/components/debates/mark_featured_action_component.rb new file mode 100644 index 000000000..38e5e59da --- /dev/null +++ b/app/components/debates/mark_featured_action_component.rb @@ -0,0 +1,12 @@ +class Debates::MarkFeaturedActionComponent < ApplicationComponent + attr_reader :debate + use_helpers :can? + + def initialize(debate) + @debate = debate + end + + def render + can? :mark_featured, debate + end +end diff --git a/app/views/debates/_actions.html.erb b/app/views/debates/_actions.html.erb index d3e64148b..99a4f7931 100644 --- a/app/views/debates/_actions.html.erb +++ b/app/views/debates/_actions.html.erb @@ -1,18 +1,2 @@ <%= render Shared::ModerationActionsComponent.new(debate) %> - -<% if can? :mark_featured, debate %> -  |  - <% if debate.featured? %> - <%= link_to t("admin.actions.unmark_featured").capitalize, unmark_featured_debate_path(debate), - method: :put, - data: { confirm: t("admin.actions.confirm_action", - action: t("admin.actions.unmark_featured"), - name: debate.title) } %> - <% else %> - <%= link_to t("admin.actions.mark_featured").capitalize, mark_featured_debate_path(debate), - method: :put, - data: { confirm: t("admin.actions.confirm_action", - action: t("admin.actions.mark_featured"), - name: debate.title) } %> - <% end %> -<% end %> +<%= render Debates::MarkFeaturedActionComponent.new(debate) %> From 68744f110ecb6d94bc12c3df2a74d77af68b6d51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 17:48:52 +0200 Subject: [PATCH 12/14] Use CSS to add separators to the debates featured action Just like we do in the moderation actions displayed next to id. --- .../debates/mark_featured_action.scss | 7 ++++ .../mark_featured_action_component.html.erb | 33 ++++++++++--------- 2 files changed, 24 insertions(+), 16 deletions(-) create mode 100644 app/assets/stylesheets/debates/mark_featured_action.scss diff --git a/app/assets/stylesheets/debates/mark_featured_action.scss b/app/assets/stylesheets/debates/mark_featured_action.scss new file mode 100644 index 000000000..f43861a6b --- /dev/null +++ b/app/assets/stylesheets/debates/mark_featured_action.scss @@ -0,0 +1,7 @@ +.debate-show .mark-featured-action { + display: inline; + + &::before { + @include vertical-separator; + } +} diff --git a/app/components/debates/mark_featured_action_component.html.erb b/app/components/debates/mark_featured_action_component.html.erb index 3eb392fcf..9e2023e42 100644 --- a/app/components/debates/mark_featured_action_component.html.erb +++ b/app/components/debates/mark_featured_action_component.html.erb @@ -1,16 +1,17 @@ - |  -<% if debate.featured? %> - <%= link_to t("admin.actions.unmark_featured").capitalize, - unmark_featured_debate_path(debate), - method: :put, - data: { confirm: t("admin.actions.confirm_action", - action: t("admin.actions.unmark_featured"), - name: debate.title) } %> -<% else %> - <%= link_to t("admin.actions.mark_featured").capitalize, - mark_featured_debate_path(debate), - method: :put, - data: { confirm: t("admin.actions.confirm_action", - action: t("admin.actions.mark_featured"), - name: debate.title) } %> -<% end %> +
+ <% if debate.featured? %> + <%= link_to t("admin.actions.unmark_featured").capitalize, + unmark_featured_debate_path(debate), + method: :put, + data: { confirm: t("admin.actions.confirm_action", + action: t("admin.actions.unmark_featured"), + name: debate.title) } %> + <% else %> + <%= link_to t("admin.actions.mark_featured").capitalize, + mark_featured_debate_path(debate), + method: :put, + data: { confirm: t("admin.actions.confirm_action", + action: t("admin.actions.mark_featured"), + name: debate.title) } %> + <% end %> +
From 87a5dd8ee5a016c198cc3fd1090582edc40dcefe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2024 05:06:22 +0200 Subject: [PATCH 13/14] Use a button to mark debates as featured As mentioned in commit 5311daadf, there are several reasons to use buttons in these situations. And, as mentioned in the previous commits, using buttons instead of links for actions requiring confirmation will help us test for accessibility issues. --- .../debates/mark_featured_action.scss | 8 +++++++ .../mark_featured_action_component.html.erb | 24 +++++++++---------- spec/system/debates_spec.rb | 4 ++-- 3 files changed, 22 insertions(+), 14 deletions(-) diff --git a/app/assets/stylesheets/debates/mark_featured_action.scss b/app/assets/stylesheets/debates/mark_featured_action.scss index f43861a6b..3684c90b8 100644 --- a/app/assets/stylesheets/debates/mark_featured_action.scss +++ b/app/assets/stylesheets/debates/mark_featured_action.scss @@ -4,4 +4,12 @@ &::before { @include vertical-separator; } + + form { + display: inline; + } + + button { + @include link; + } } diff --git a/app/components/debates/mark_featured_action_component.html.erb b/app/components/debates/mark_featured_action_component.html.erb index 9e2023e42..5d7fe6565 100644 --- a/app/components/debates/mark_featured_action_component.html.erb +++ b/app/components/debates/mark_featured_action_component.html.erb @@ -1,17 +1,17 @@
<% if debate.featured? %> - <%= link_to t("admin.actions.unmark_featured").capitalize, - unmark_featured_debate_path(debate), - method: :put, - data: { confirm: t("admin.actions.confirm_action", - action: t("admin.actions.unmark_featured"), - name: debate.title) } %> + <%= button_to t("admin.actions.unmark_featured").capitalize, + unmark_featured_debate_path(debate), + method: :put, + data: { confirm: t("admin.actions.confirm_action", + action: t("admin.actions.unmark_featured"), + name: debate.title) } %> <% else %> - <%= link_to t("admin.actions.mark_featured").capitalize, - mark_featured_debate_path(debate), - method: :put, - data: { confirm: t("admin.actions.confirm_action", - action: t("admin.actions.mark_featured"), - name: debate.title) } %> + <%= button_to t("admin.actions.mark_featured").capitalize, + mark_featured_debate_path(debate), + method: :put, + data: { confirm: t("admin.actions.confirm_action", + action: t("admin.actions.mark_featured"), + name: debate.title) } %> <% end %>
diff --git a/spec/system/debates_spec.rb b/spec/system/debates_spec.rb index ff1b499d5..7496e10af 100644 --- a/spec/system/debates_spec.rb +++ b/spec/system/debates_spec.rb @@ -748,7 +748,7 @@ describe "Debates" do end click_link debate.title - accept_confirm("Are you sure? Featured") { click_link "Featured" } + accept_confirm("Are you sure? Featured") { click_button "Featured" } within("#debates") do expect(page).to have_content "FEATURED" @@ -760,7 +760,7 @@ describe "Debates" do click_link debate.title end - accept_confirm("Are you sure? Unmark featured") { click_link "Unmark featured" } + accept_confirm("Are you sure? Unmark featured") { click_button "Unmark featured" } within("#debates") do expect(page).not_to have_content "FEATURED" From ef4bc6a650bdcc3abdab15ad1ae293d124abb40f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 8 Nov 2024 13:35:22 +0100 Subject: [PATCH 14/14] Simplify `accept_confirm` dialog in valuation test We were using `check "Valuation finished"` everywhere except here. This way it's easier to realize, while reading the test, that we're interacting with a checkbox and not a link or a button. --- spec/system/valuation/budget_investments_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/system/valuation/budget_investments_spec.rb b/spec/system/valuation/budget_investments_spec.rb index 3d542abb1..7b0727730 100644 --- a/spec/system/valuation/budget_investments_spec.rb +++ b/spec/system/valuation/budget_investments_spec.rb @@ -410,7 +410,7 @@ describe "Valuation budget investments" do visit valuation_budget_budget_investment_path(budget, investment) click_link "Edit dossier" - accept_confirm { find_field("budget_investment[valuation_finished]").click } + accept_confirm { check "Valuation finished" } click_button "Save changes" expect(page).to have_content "Dossier updated"