From 75ef3e0a511110338a230cd1fde6bf26402621fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 3 Apr 2024 23:32:16 +0200 Subject: [PATCH 01/15] Avoid empty links in banners without content This happened when previewing banners in the "new banner form", which might cause accessibility issues when people access the list of links on the page. We were getting the following accessibility error: ``` link-name: Links must have discernible text (serious) https://dequeuniversity.com/rules/axe/4.9/link-name?application=axeAPI The following node violate this rule: Selector: a[href$="new"] HTML:

Fix all of the following: - Element is in tab order and does not have accessible text Fix any of the following: - Element does not have text that is visible to screen readers - aria-label attribute does not exist or is empty - aria-labelledby attribute does not exist, references elements that do not exist or references elements that - Element has no title attribute ``` --- app/components/shared/banner_component.html.erb | 8 +++----- app/components/shared/banner_component.rb | 4 ++++ spec/components/shared/banner_component_spec.rb | 8 ++++++++ 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/app/components/shared/banner_component.html.erb b/app/components/shared/banner_component.html.erb index fe020302a..cbea18d0e 100644 --- a/app/components/shared/banner_component.html.erb +++ b/app/components/shared/banner_component.html.erb @@ -1,5 +1,3 @@ -<% if banner %> - -<% end %> + diff --git a/app/components/shared/banner_component.rb b/app/components/shared/banner_component.rb index de61e1602..21aba7c65 100644 --- a/app/components/shared/banner_component.rb +++ b/app/components/shared/banner_component.rb @@ -13,6 +13,10 @@ class Shared::BannerComponent < ApplicationComponent end end + def render? + banner && (banner.title.present? || banner.description.present?) + end + private def link diff --git a/spec/components/shared/banner_component_spec.rb b/spec/components/shared/banner_component_spec.rb index 6d2512373..403a45e9d 100644 --- a/spec/components/shared/banner_component_spec.rb +++ b/spec/components/shared/banner_component_spec.rb @@ -31,6 +31,14 @@ describe Shared::BannerComponent do expect(page).not_to have_content "Proposal banner" end + it "does not render an empty banner" do + banner = build(:banner, title: "", description: "") + + render_inline Shared::BannerComponent.new(banner) + + expect(page).not_to be_rendered + end + context "several banners available in the same section" do before do create(:banner, From db9b849d8295c5bd45b2c614fc180abcc8a68a3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 27 Feb 2025 13:36:57 +0100 Subject: [PATCH 02/15] Use labels to fill in fields in management account tests This way we're testing that the label is correctly associated with the field. --- spec/system/management/account_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/system/management/account_spec.rb b/spec/system/management/account_spec.rb index 9350137db..0507affe1 100644 --- a/spec/system/management/account_spec.rb +++ b/spec/system/management/account_spec.rb @@ -34,7 +34,7 @@ describe "Account" do login_as_manager click_link "Reset password manually" - find(:css, "input[id$='user_password']").set("new_password") + fill_in "Password", with: "new_password" click_button "Save password" @@ -75,7 +75,7 @@ describe "Account" do login_as_manager click_link "Reset password manually" - find(:css, "input[id$='user_password']").set("another_new_password") + fill_in "Password", with: "another_new_password" click_button "Save password" From 248b826f20c6556ecd2a47ca9d803f318ac1ff4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 27 Feb 2025 13:39:41 +0100 Subject: [PATCH 03/15] Merge reset password tests together Since system tests are slow, and these tests were almost identical, we're merging them into one to make them faster. --- spec/system/management/account_spec.rb | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-) diff --git a/spec/system/management/account_spec.rb b/spec/system/management/account_spec.rb index 0507affe1..6802a711d 100644 --- a/spec/system/management/account_spec.rb +++ b/spec/system/management/account_spec.rb @@ -27,7 +27,7 @@ describe "Account" do expect(email).to have_text "Change your password" end - scenario "Manager changes the password by hand (writen by them)" do + scenario "Manager manually writes the new password" do user = create(:user, :level_three) login_managed_user(user) @@ -39,6 +39,8 @@ describe "Account" do click_button "Save password" expect(page).to have_content "Password reseted successfully" + expect(page).to have_link "Print password", href: "javascript:window.print();" + expect(page).to have_css "div.for-print-only", text: "new_password", visible: :hidden logout @@ -68,22 +70,6 @@ describe "Account" do expect(page).to have_content "You have been signed in successfully." end - scenario "The password is printed" do - user = create(:user, :level_three) - login_managed_user(user) - - login_as_manager - click_link "Reset password manually" - - fill_in "Password", with: "another_new_password" - - click_button "Save password" - - expect(page).to have_content "Password reseted successfully" - expect(page).to have_link "Print password", href: "javascript:window.print();" - expect(page).to have_css("div.for-print-only", text: "another_new_password", visible: :hidden) - end - describe "When a user has not been selected" do before do Setting["feature.user.skip_verification"] = "true" From a1bdaf6e8f07cd683c2de2438f980d86e76cd5e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 08:53:29 +0100 Subject: [PATCH 04/15] Add a text to the show password link in management area We were using an icon for this link, but people who can't see the icon couldn't know what the link was about. Axe was reporting the following accessibility error: ``` link-name: Links must have discernible text (serious) https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI The following 1 node violate this rule: Selector: .show-password HTML: Fix all of the following: - Element is in tab order and does not have accessible text Fix any of the following: - Element does not have text that is visible to screen readers - aria-label attribute does not exist or is empty - aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty - Element has no title attribute ``` --- .../account/edit_password_manually_component.html.erb | 5 ++++- config/locales/en/management.yml | 1 + config/locales/es/management.yml | 1 + spec/system/management/account_spec.rb | 10 ++++++++++ 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/app/components/management/account/edit_password_manually_component.html.erb b/app/components/management/account/edit_password_manually_component.html.erb index 484974695..c59848e62 100644 --- a/app/components/management/account/edit_password_manually_component.html.erb +++ b/app/components/management/account/edit_password_manually_component.html.erb @@ -10,7 +10,10 @@
<%= f.password_field :password, class: "input-group-field", label: false, value: nil %> - + + + <%= t("management.account.edit.password.show") %> +
diff --git a/config/locales/en/management.yml b/config/locales/en/management.yml index 3cf7506a3..06e9ab967 100644 --- a/config/locales/en/management.yml +++ b/config/locales/en/management.yml @@ -18,6 +18,7 @@ en: reseted: Password reseted successfully random: Generate random password save: Save password + show: Show password print: Print password print_help: You will be able to print the password when it is saved. account_info: diff --git a/config/locales/es/management.yml b/config/locales/es/management.yml index 895268f16..e7510f6cc 100644 --- a/config/locales/es/management.yml +++ b/config/locales/es/management.yml @@ -18,6 +18,7 @@ es: reseted: Contraseña restablecida correctamente random: Generar contraseña aleatoria save: Guardar contraseña + show: Mostrar contraseña print: Imprimir contraseña print_help: Podrás imprimir la contraseña cuando se haya guardado. account_info: diff --git a/spec/system/management/account_spec.rb b/spec/system/management/account_spec.rb index 6802a711d..b2d219de0 100644 --- a/spec/system/management/account_spec.rb +++ b/spec/system/management/account_spec.rb @@ -59,6 +59,16 @@ describe "Account" do new_password = find_field("user_password").value + expect(page).to have_field "Password", type: :password + + click_link "Show password" + + expect(page).to have_field "Password", type: :text + + click_link "Show password" + + expect(page).to have_field "Password", type: :password + click_button "Save password" expect(page).to have_content "Password reseted successfully" From 5df85bac4e84b6439bf4805d91aa4fcb2dd40af5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 09:00:47 +0100 Subject: [PATCH 05/15] Fix margin in password field when showing the password We were applying a rule that wasn't taking effect after clicking the "show password" link. --- .../stylesheets/management/account/edit_password_manually.scss | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/management/account/edit_password_manually.scss b/app/assets/stylesheets/management/account/edit_password_manually.scss index 581fd2579..6b5613e36 100644 --- a/app/assets/stylesheets/management/account/edit_password_manually.scss +++ b/app/assets/stylesheets/management/account/edit_password_manually.scss @@ -1,5 +1,6 @@ .management-account-edit-password-manually { - [type=password] { + [type=password], + [type=text] { margin-bottom: 0 !important; } } From 573f0e62cc9d1a1bb2319aa534d6f4b7c198e978 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 08:57:30 +0100 Subject: [PATCH 06/15] Use a button to show/hide password in the management area When using a link, people using screen readers might think they're going to a new page where the password is going to be shown. With a button, they get a better idea about what to expect. Furthermore, with a button, we can use the `aria-pressed` attribute to indicate whether the password is currently being shown. --- app/assets/javascripts/managers.js | 5 ++--- .../management/account/edit_password_manually.scss | 4 ++++ .../account/edit_password_manually_component.html.erb | 4 ++-- spec/system/management/account_spec.rb | 7 +++++-- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/managers.js b/app/assets/javascripts/managers.js index 78774bb29..2926c8e0a 100644 --- a/app/assets/javascripts/managers.js +++ b/app/assets/javascripts/managers.js @@ -39,14 +39,13 @@ e.stopPropagation(); $("#user_password").val(App.Managers.generatePassword()); }); - $(".show-password").on("click", function(e) { - e.preventDefault(); - e.stopPropagation(); + $(".show-password").on("click", function() { if ($("#user_password").is("input[type='password']")) { App.Managers.togglePassword("text"); } else { App.Managers.togglePassword("password"); } + $(this).attr("aria-pressed", !JSON.parse($(this).attr("aria-pressed"))); }); } }; diff --git a/app/assets/stylesheets/management/account/edit_password_manually.scss b/app/assets/stylesheets/management/account/edit_password_manually.scss index 6b5613e36..4d8aaa3ce 100644 --- a/app/assets/stylesheets/management/account/edit_password_manually.scss +++ b/app/assets/stylesheets/management/account/edit_password_manually.scss @@ -3,4 +3,8 @@ [type=text] { margin-bottom: 0 !important; } + + .show-password { + cursor: pointer; + } } diff --git a/app/components/management/account/edit_password_manually_component.html.erb b/app/components/management/account/edit_password_manually_component.html.erb index c59848e62..9887cbcac 100644 --- a/app/components/management/account/edit_password_manually_component.html.erb +++ b/app/components/management/account/edit_password_manually_component.html.erb @@ -10,10 +10,10 @@
<%= f.password_field :password, class: "input-group-field", label: false, value: nil %> - +
diff --git a/spec/system/management/account_spec.rb b/spec/system/management/account_spec.rb index b2d219de0..100c3502c 100644 --- a/spec/system/management/account_spec.rb +++ b/spec/system/management/account_spec.rb @@ -60,14 +60,17 @@ describe "Account" do new_password = find_field("user_password").value expect(page).to have_field "Password", type: :password + expect(page).to have_css "button[aria-pressed=false]", exact_text: "Show password" - click_link "Show password" + click_button "Show password" expect(page).to have_field "Password", type: :text + expect(page).to have_css "button[aria-pressed=true]", exact_text: "Show password" - click_link "Show password" + click_button "Show password" expect(page).to have_field "Password", type: :password + expect(page).to have_css "button[aria-pressed=false]", exact_text: "Show password" click_button "Save password" From 5f9c6fd554d4d90b87b1bdcb2cea49d917e27f2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 09:03:01 +0100 Subject: [PATCH 07/15] Use an SVG icon for the show password button As mentioned in commit 925f04e3f, SVG icons offer many advantages over icon fonts. --- .../stylesheets/management/account/edit_password_manually.scss | 1 + .../management/account/edit_password_manually_component.html.erb | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/management/account/edit_password_manually.scss b/app/assets/stylesheets/management/account/edit_password_manually.scss index 4d8aaa3ce..21c1b4900 100644 --- a/app/assets/stylesheets/management/account/edit_password_manually.scss +++ b/app/assets/stylesheets/management/account/edit_password_manually.scss @@ -5,6 +5,7 @@ } .show-password { + @include has-fa-icon(eye, regular); cursor: pointer; } } diff --git a/app/components/management/account/edit_password_manually_component.html.erb b/app/components/management/account/edit_password_manually_component.html.erb index 9887cbcac..497145ec2 100644 --- a/app/components/management/account/edit_password_manually_component.html.erb +++ b/app/components/management/account/edit_password_manually_component.html.erb @@ -11,7 +11,6 @@ <%= f.password_field :password, class: "input-group-field", label: false, value: nil %> From 942ccf66745cfd0af98e7def83174d7838e82400 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 27 Feb 2025 14:35:41 +0100 Subject: [PATCH 08/15] Group similar dashboard progress tests together All these tests were basically checking the same things. Since system tests are slow, we're grouping them together so executing them is slightly faster. --- spec/system/dashboard/dashboard_spec.rb | 26 ++----------------------- 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/spec/system/dashboard/dashboard_spec.rb b/spec/system/dashboard/dashboard_spec.rb index aaf7cfb28..1550361de 100644 --- a/spec/system/dashboard/dashboard_spec.rb +++ b/spec/system/dashboard/dashboard_spec.rb @@ -47,14 +47,6 @@ describe "Proposal's dashboard" do end end - scenario "Dashboard progress show proposed actions" do - action = create(:dashboard_action, :proposed_action, :active) - - visit progress_proposal_dashboard_path(proposal) - - expect(page).to have_content(action.title) - end - scenario "Dashboard progress show proposed actions truncated description" do action = create(:dashboard_action, :proposed_action, :active, description: "One short action") action_long = create(:dashboard_action, :proposed_action, :active, @@ -105,28 +97,14 @@ describe "Proposal's dashboard" do visit progress_proposal_dashboard_path(proposal) within "#proposed_actions_pending" do - expect(page).to have_content(action.title) + expect(page).to have_content action.title end - end - scenario "Dashboard progress display proposed_action done on his section" do - action = create(:dashboard_action, :proposed_action, :active) - - visit progress_proposal_dashboard_path(proposal) find(:css, "#dashboard_action_#{action.id}_execute").click within "#proposed_actions_done" do - expect(page).to have_content(action.title) + expect(page).to have_content action.title end - end - - scenario "Dashboard progress can execute proposed action" do - action = create(:dashboard_action, :proposed_action, :active) - - visit progress_proposal_dashboard_path(proposal) - expect(page).to have_content(action.title) - - find(:css, "#dashboard_action_#{action.id}_execute").click expect(page).not_to have_css "#dashboard_action_#{action.id}_execute" end From 4e08f3f14705392bd070b5789664e05e2a3cb1fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 09:21:35 +0100 Subject: [PATCH 09/15] Add a text to the links to execute dashboard actions People using screen readers had no idea what these links were about (not that the icons are very usable for people seeing them either... but that's a different topic). Axe was reporting this error: ``` link-name: Links must have discernible text (serious) https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI The following 1 node violate this rule: Selector: #dashboard_action_2_execute HTML: Fix all of the following: - Element is in tab order and does not have accessible text Fix any of the following: - Element does not have text that is visible to screen readers - aria-label attribute does not exist or is empty - aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty - Element has no title attribute ``` --- app/views/dashboard/_proposed_action.html.erb | 8 +++-- config/locales/en/general.yml | 2 ++ config/locales/es/general.yml | 2 ++ spec/system/dashboard/dashboard_spec.rb | 33 ++++++++++++------- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/app/views/dashboard/_proposed_action.html.erb b/app/views/dashboard/_proposed_action.html.erb index 082ddf0ed..0a593ad8f 100644 --- a/app/views/dashboard/_proposed_action.html.erb +++ b/app/views/dashboard/_proposed_action.html.erb @@ -2,16 +2,20 @@
<% if proposed_action.proposals.where(id: proposal.id).any? %> <%= link_to unexecute_proposal_dashboard_action_path(proposal, proposed_action), - id: "#{dom_id(proposed_action)}_unexecute", method: :post, class: "checked-link" do %> + + <%= t("dashboard.recommended_actions.unexecute", name: proposed_action.title) %> + <% end %> <% else %> <%= link_to execute_proposal_dashboard_action_path(proposal, proposed_action), - id: "#{dom_id(proposed_action)}_execute", method: :post, class: "unchecked-link" do %> + + <%= t("dashboard.recommended_actions.execute", name: proposed_action.title) %> + <% end %> <% end %> diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 4ee75c137..163091a05 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -475,6 +475,8 @@ en: see_proposed_actions: Check out recommended actions hide_proposed_actions: Hide recommended actions pending_title: Pending + execute: "Mark %{name} as done" + unexecute: "Unmark %{name} as done" show_description: Show description without_pending_actions: No recommended actions pending done_title: Done diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 458f437a8..1b803c460 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -475,6 +475,8 @@ es: see_proposed_actions: Ver todas las acciones recomendadas hide_proposed_actions: Ocultar acciones recomendadas pending_title: Pendientes + execute: "Marcar la acción %{name} como realizada" + unexecute: "Desmarcar la acción %{name} como realizada" show_description: Mostrar descripción without_pending_actions: No hay acciones recomendadas pendientes done_title: Realizadas diff --git a/spec/system/dashboard/dashboard_spec.rb b/spec/system/dashboard/dashboard_spec.rb index 1550361de..fc2c6416c 100644 --- a/spec/system/dashboard/dashboard_spec.rb +++ b/spec/system/dashboard/dashboard_spec.rb @@ -92,31 +92,42 @@ describe "Proposal's dashboard" do end scenario "Dashboard progress display proposed_action pending on his section" do - action = create(:dashboard_action, :proposed_action, :active) + create(:dashboard_action, :proposed_action, :active, title: "Expand!") visit progress_proposal_dashboard_path(proposal) within "#proposed_actions_pending" do - expect(page).to have_content action.title + expect(page).to have_content "Expand!" end - find(:css, "#dashboard_action_#{action.id}_execute").click + click_link "Mark Expand! as done" within "#proposed_actions_done" do - expect(page).to have_content action.title + expect(page).to have_content "Expand!" end - expect(page).not_to have_css "#dashboard_action_#{action.id}_execute" + + expect(page).not_to have_link "Mark Expand! as done" + expect(page).to have_link "Unmark Expand! as done" end scenario "Dashboard progress can unexecute proposed action" do - action = create(:dashboard_action, :proposed_action, :active) + action = create(:dashboard_action, :proposed_action, :active, title: "Reinforce!") create(:dashboard_executed_action, proposal: proposal, action: action) visit progress_proposal_dashboard_path(proposal) - expect(page).to have_content(action.title) - find(:css, "#dashboard_action_#{action.id}_unexecute").click - expect(page).to have_css "#dashboard_action_#{action.id}_execute" + within "#proposed_actions_done" do + expect(page).to have_content "Reinforce!" + end + + click_link "Unmark Reinforce! as done" + + within "#proposed_actions_pending" do + expect(page).to have_content "Reinforce!" + end + + expect(page).not_to have_link "Unmark Reinforce! as done" + expect(page).to have_link "Mark Reinforce! as done" end scenario "Dashboard progress dont show proposed actions with published_proposal: true" do @@ -450,10 +461,10 @@ describe "Proposal's dashboard" do end scenario "On recommended actions section display proposed_action done on his section" do - action = create(:dashboard_action, :proposed_action, :active) + action = create(:dashboard_action, :proposed_action, :active, title: "Make progress") visit recommended_actions_proposal_dashboard_path(proposal.to_param) - find(:css, "#dashboard_action_#{action.id}_execute").click + click_link "Mark Make progress as done" within "#proposed_actions_done" do expect(page).to have_content(action.title) From 6c3e7391d4647ff7467c7aa189a8c48b5d252bd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 10:48:22 +0100 Subject: [PATCH 10/15] Use buttons to execute/unexecute dashboard actions As mentioned in commit 5311daadf, using buttons for non-GET requests has several advantages over using links. --- app/assets/stylesheets/dashboard.scss | 6 +++++- app/views/dashboard/_proposed_action.html.erb | 10 ++++------ spec/system/dashboard/dashboard_spec.rb | 14 +++++++------- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/app/assets/stylesheets/dashboard.scss b/app/assets/stylesheets/dashboard.scss index 5bc379bee..a6b989b4a 100644 --- a/app/assets/stylesheets/dashboard.scss +++ b/app/assets/stylesheets/dashboard.scss @@ -139,6 +139,11 @@ } } + form { + display: inline; + vertical-align: top; + } + .icon-check { display: inline-block; font-size: rem-calc(24); @@ -147,7 +152,6 @@ .unchecked-link { display: inline-block; - vertical-align: top; } .unchecked { diff --git a/app/views/dashboard/_proposed_action.html.erb b/app/views/dashboard/_proposed_action.html.erb index 0a593ad8f..fd7de8adb 100644 --- a/app/views/dashboard/_proposed_action.html.erb +++ b/app/views/dashboard/_proposed_action.html.erb @@ -1,18 +1,16 @@
<% if proposed_action.proposals.where(id: proposal.id).any? %> - <%= link_to unexecute_proposal_dashboard_action_path(proposal, proposed_action), - method: :post, - class: "checked-link" do %> + <%= button_to unexecute_proposal_dashboard_action_path(proposal, proposed_action), + class: "checked-link" do %> <%= t("dashboard.recommended_actions.unexecute", name: proposed_action.title) %> <% end %> <% else %> - <%= link_to execute_proposal_dashboard_action_path(proposal, proposed_action), - method: :post, - class: "unchecked-link" do %> + <%= button_to execute_proposal_dashboard_action_path(proposal, proposed_action), + class: "unchecked-link" do %> <%= t("dashboard.recommended_actions.execute", name: proposed_action.title) %> diff --git a/spec/system/dashboard/dashboard_spec.rb b/spec/system/dashboard/dashboard_spec.rb index fc2c6416c..b318c1dc7 100644 --- a/spec/system/dashboard/dashboard_spec.rb +++ b/spec/system/dashboard/dashboard_spec.rb @@ -100,14 +100,14 @@ describe "Proposal's dashboard" do expect(page).to have_content "Expand!" end - click_link "Mark Expand! as done" + click_button "Mark Expand! as done" within "#proposed_actions_done" do expect(page).to have_content "Expand!" end - expect(page).not_to have_link "Mark Expand! as done" - expect(page).to have_link "Unmark Expand! as done" + expect(page).not_to have_button "Mark Expand! as done" + expect(page).to have_button "Unmark Expand! as done" end scenario "Dashboard progress can unexecute proposed action" do @@ -120,14 +120,14 @@ describe "Proposal's dashboard" do expect(page).to have_content "Reinforce!" end - click_link "Unmark Reinforce! as done" + click_button "Unmark Reinforce! as done" within "#proposed_actions_pending" do expect(page).to have_content "Reinforce!" end - expect(page).not_to have_link "Unmark Reinforce! as done" - expect(page).to have_link "Mark Reinforce! as done" + expect(page).not_to have_button "Unmark Reinforce! as done" + expect(page).to have_button "Mark Reinforce! as done" end scenario "Dashboard progress dont show proposed actions with published_proposal: true" do @@ -464,7 +464,7 @@ describe "Proposal's dashboard" do action = create(:dashboard_action, :proposed_action, :active, title: "Make progress") visit recommended_actions_proposal_dashboard_path(proposal.to_param) - click_link "Mark Make progress as done" + click_button "Mark Make progress as done" within "#proposed_actions_done" do expect(page).to have_content(action.title) From d8550388a68b02d31658ce67543ec699b6951153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 10:56:32 +0100 Subject: [PATCH 11/15] Use SVG icons for the execute action buttons This way we simplify the CSS and, in the case of the "check" icon, using an SVG icon instead of an icon font offers several advantages, as mentioned in commit 925f04e3f. --- app/assets/stylesheets/dashboard.scss | 23 ++++++++----------- app/views/dashboard/_proposed_action.html.erb | 2 -- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/app/assets/stylesheets/dashboard.scss b/app/assets/stylesheets/dashboard.scss index a6b989b4a..7bf1b02f1 100644 --- a/app/assets/stylesheets/dashboard.scss +++ b/app/assets/stylesheets/dashboard.scss @@ -142,25 +142,20 @@ form { display: inline; vertical-align: top; + + button { + font-size: 1.5em; + } } - .icon-check { - display: inline-block; - font-size: rem-calc(24); - vertical-align: top; + .checked-link { + @include has-fa-icon(check, solid); + color: $check; } .unchecked-link { - display: inline-block; - } - - .unchecked { - border: 1px solid $border; - border-radius: rem-calc(4); - display: inline-block; - height: rem-calc(20); - margin-top: calc($line-height / 6); - width: rem-calc(20); + @include has-fa-icon(square, regular); + color: $border; } } diff --git a/app/views/dashboard/_proposed_action.html.erb b/app/views/dashboard/_proposed_action.html.erb index fd7de8adb..1a351829c 100644 --- a/app/views/dashboard/_proposed_action.html.erb +++ b/app/views/dashboard/_proposed_action.html.erb @@ -6,7 +6,6 @@ <%= t("dashboard.recommended_actions.unexecute", name: proposed_action.title) %> - <% end %> <% else %> <%= button_to execute_proposal_dashboard_action_path(proposal, proposed_action), @@ -14,7 +13,6 @@ <%= t("dashboard.recommended_actions.execute", name: proposed_action.title) %> - <% end %> <% end %>
From d29fca116296f1112949a8a2ad164c246246d971 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 10:59:54 +0100 Subject: [PATCH 12/15] Fix contrast in buttons to execute dashboard actions The colors we were using didn't meet the minimum contrast required for UI elements. --- app/assets/stylesheets/dashboard.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/dashboard.scss b/app/assets/stylesheets/dashboard.scss index 7bf1b02f1..4c819c306 100644 --- a/app/assets/stylesheets/dashboard.scss +++ b/app/assets/stylesheets/dashboard.scss @@ -150,12 +150,12 @@ .checked-link { @include has-fa-icon(check, solid); - color: $check; + color: $color-success; } .unchecked-link { @include has-fa-icon(square, regular); - color: $border; + color: $dark-gray; } } From da53a6acae328eac8e9676a79accc06f5339a346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 11:40:35 +0100 Subject: [PATCH 13/15] Validate result publication enabled processes have a date Just like we do with the rest of the phases. The reason why we're making this change right now is that we were getting an accessibility error with processes with no result publication date: ``` link-name: Links must have discernible text (serious) https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI The following 1 node violate this rule: Selector: p:nth-child(6) > a HTML: Fix all of the following: - Element is in tab order and does not have accessible text Fix any of the following: - Element does not have text that is visible to screen readers - aria-label attribute does not exist or is empty - aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty - Element has no title attribute ``` --- app/models/legislation/process.rb | 1 + spec/models/legislation/process_spec.rb | 15 +++++++++++++++ spec/system/legislation/summary_spec.rb | 1 + 3 files changed, 17 insertions(+) diff --git a/app/models/legislation/process.rb b/app/models/legislation/process.rb index f59ddf377..d4cf5284f 100644 --- a/app/models/legislation/process.rb +++ b/app/models/legislation/process.rb @@ -46,6 +46,7 @@ class Legislation::Process < ApplicationRecord validates_translation :title, presence: true validates :start_date, presence: true validates :end_date, presence: true + validates :result_publication_date, presence: true, if: :result_publication_enabled? %i[draft debate proposals_phase allegations].each do |phase_name| enabled_attribute = :"#{phase_name.to_s.gsub("_phase", "")}_phase_enabled?" diff --git a/spec/models/legislation/process_spec.rb b/spec/models/legislation/process_spec.rb index 5b62b98f4..511e5aa62 100644 --- a/spec/models/legislation/process_spec.rb +++ b/spec/models/legislation/process_spec.rb @@ -154,6 +154,21 @@ describe Legislation::Process do expect(proposals_disabled).to be_valid expect(allegations_disabled).to be_valid end + + it "is invalid if result publication is enabled but result_publication_date is not present" do + process = build(:legislation_process, result_publication_enabled: true, result_publication_date: "") + + expect(process).not_to be_valid + expect(process.errors.messages[:result_publication_date]).to include("can't be blank") + end + + it "is valid if result publication is enabled and result_publication_date is present" do + process = build(:legislation_process, + result_publication_enabled: true, + result_publication_date: Date.tomorrow) + + expect(process).to be_valid + end end describe "date ranges validations" do diff --git a/spec/system/legislation/summary_spec.rb b/spec/system/legislation/summary_spec.rb index 36c9ffe28..a521d0827 100644 --- a/spec/system/legislation/summary_spec.rb +++ b/spec/system/legislation/summary_spec.rb @@ -22,6 +22,7 @@ describe "Legislation" do scenario "empty process" do process = create(:legislation_process, :empty, result_publication_enabled: true, + result_publication_date: 1.day.ago, end_date: Date.current - 1.day) visit summary_legislation_process_path(process) From 2f667a6225f1dcccc556c0dc1f1c269264b68987 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 12:38:57 +0100 Subject: [PATCH 14/15] Add missing expectation to annotations test This expectations in this test were true both before and after clicking on the `.icon-expand` link, so it was possible that the test finished before the request generated by that click did. So we're adding an extra expectation to make sure we're testing what we want to test: the content of the page after the request has finished. --- spec/system/comments/legislation_annotations_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/system/comments/legislation_annotations_spec.rb b/spec/system/comments/legislation_annotations_spec.rb index 22d169430..6fbab0f7a 100644 --- a/spec/system/comments/legislation_annotations_spec.rb +++ b/spec/system/comments/legislation_annotations_spec.rb @@ -39,9 +39,10 @@ describe "Commenting legislation annotations" do find(".icon-expand").click end - expect(page).to have_css(".comment", count: 2) - expect(page).to have_content("my annotation") - expect(page).to have_content("my other annotation") + expect(page).to have_content "Comments about" + expect(page).to have_css ".comment", count: 2 + expect(page).to have_content "my annotation" + expect(page).to have_content "my other annotation" end scenario "Reply on a single annotation thread and display it in the merged annotation thread" do From b08c279c314d3bf9bd9e0425d98564226bc15442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Wed, 26 Feb 2025 12:15:10 +0100 Subject: [PATCH 15/15] Make links to annotation comments accessible The link to the comments page was an "expand" icon, which was confusing because it wasn't really expanding the contents of the sidebar but going to an entirely different page. Furthermore, it didn't have any text for people using screen readers, which is why Axe was reporting the following accessibility error: ``` link-name: Links must have discernible text (serious) https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI The following 1 node violate this rule: Selector: #annotation-link > a HTML: Fix all of the following: - Element is in tab order and does not have accessible text Fix any of the following: - Element does not have text that is visible to screen readers - aria-label attribute does not exist or is empty - aria-labelledby attribute does not exist, references elements that do not exist or references elements that are empty - Element has no title attribute ``` So we're removing the icon and turning the "N comments" text into a link, so it's easier to guess that the link takes us to the page showing all the comments for this annotation. --- .../stylesheets/legislation_process.scss | 6 ------ .../annotations/_annotation_link.html.erb | 3 --- .../annotations/_comment_header.html.erb | 9 ++++++--- .../legislation/annotations/comments.js.erb | 4 ++-- .../comments/legislation_annotations_spec.rb | 20 +++++++------------ 5 files changed, 15 insertions(+), 27 deletions(-) delete mode 100644 app/views/legislation/annotations/_annotation_link.html.erb diff --git a/app/assets/stylesheets/legislation_process.scss b/app/assets/stylesheets/legislation_process.scss index 5e7f78fc7..b752d3688 100644 --- a/app/assets/stylesheets/legislation_process.scss +++ b/app/assets/stylesheets/legislation_process.scss @@ -596,12 +596,6 @@ border-bottom: 1px solid $border; margin-bottom: rem-calc(16); padding-bottom: rem-calc(8); - - a .icon-expand { - color: #838383; - float: right; - font-size: $small-font-size; - } } .comments-wrapper { diff --git a/app/views/legislation/annotations/_annotation_link.html.erb b/app/views/legislation/annotations/_annotation_link.html.erb deleted file mode 100644 index b047f52cb..000000000 --- a/app/views/legislation/annotations/_annotation_link.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= link_to legislation_process_draft_version_annotation_path(annotation.draft_version.process, annotation.draft_version, annotation, sub_annotation_ids: "") do %> - -<% end %> diff --git a/app/views/legislation/annotations/_comment_header.html.erb b/app/views/legislation/annotations/_comment_header.html.erb index f767cf7a3..80d55cdb9 100644 --- a/app/views/legislation/annotations/_comment_header.html.erb +++ b/app/views/legislation/annotations/_comment_header.html.erb @@ -1,5 +1,8 @@ -<%= render Shared::CommentsCountComponent.new(annotation.comments.roots.count) %> - - <%= render "annotation_link", annotation: annotation %> + <%= render Shared::CommentsCountComponent.new( + annotation.comments.roots.count, + url: legislation_process_draft_version_annotation_path( + annotation.draft_version.process, annotation.draft_version, annotation, sub_annotation_ids: "" + ) + ) %> diff --git a/app/views/legislation/annotations/comments.js.erb b/app/views/legislation/annotations/comments.js.erb index 1e0985217..46550b8cd 100644 --- a/app/views/legislation/annotations/comments.js.erb +++ b/app/views/legislation/annotations/comments.js.erb @@ -14,11 +14,11 @@ if ($(".comment").length == 0) { $("#annotation-link a").attr("href", new_annotation_link) - var current_comment_text = $(".comments-count").text() + var current_comment_text = $("#annotation-link a").text() var current_comment_count = current_comment_text.match(/\d+/)[0] var new_comment_count = parseInt(current_comment_count) + parseInt(<%= @annotation.comments.roots.count %>) var new_comment_count_text = current_comment_text.replace(/(\d+)/, new_comment_count); - $(".comments-count").text(new_comment_count_text) + $("#annotation-link a").text(new_comment_count_text) } <%= render "comments_box_form", comment: @comment, annotation: @annotation %> diff --git a/spec/system/comments/legislation_annotations_spec.rb b/spec/system/comments/legislation_annotations_spec.rb index 6fbab0f7a..d547d282e 100644 --- a/spec/system/comments/legislation_annotations_spec.rb +++ b/spec/system/comments/legislation_annotations_spec.rb @@ -35,9 +35,7 @@ describe "Commenting legislation annotations" do end scenario "View comments of annotations in an included range" do - within("#annotation-link") do - find(".icon-expand").click - end + click_link "2 comment" expect(page).to have_content "Comments about" expect(page).to have_css ".comment", count: 2 @@ -71,20 +69,16 @@ describe "Commenting legislation annotations" do expect(page).to have_content "my other annotation" end - within("#annotation-link") do - find(".icon-expand").click - end + click_link "2 comment" - expect(page).to have_css(".comment", count: 3) - expect(page).to have_content("my annotation") - expect(page).to have_content("my other annotation") - expect(page).to have_content("replying in single annotation thread") + expect(page).to have_css ".comment", count: 3 + expect(page).to have_content "my annotation" + expect(page).to have_content "my other annotation" + expect(page).to have_content "replying in single annotation thread" end scenario "Reply on a multiple annotation thread and display it in the single annotation thread" do - within("#annotation-link") do - find(".icon-expand").click - end + click_link "2 comment" within("#comment_#{comment2.id}") do click_link "Reply"