From 7a25c61ae6a0fff47638bfcba13beb71b4c134b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 30 Sep 2021 22:07:14 +0200 Subject: [PATCH] Use buttons to vote in the final voting As mentioned in commits 5311daadf and bb958daf0, using links combined with JavaScript to generate POST requests to the server has a few issues. We're also improving the keyboard access. Previously, the links were focusable and clickable with the keyboard. Now we're disabling the buttons when voting isn't allowed. Since these elements can no longer be focused, we're adding an element with `tabindex="0"` so the "participation not allowed" message is shown, like we do in most places. Note we're slightly changing one test because now when hovering over the button on Chrome, the "participation not allowed" text isn't shown; it's only shown when hovering on the parts of the `div.ballot` element outside the button. Since we're already rewriting the behavior of the "participation not allowed" text in a different pull request, we aren't going to fix this behavior. --- .../investments/ballot_component.html.erb | 59 ++++++++++--------- .../investments/ballot_component_spec.rb | 22 +++---- spec/support/common_actions/budgets.rb | 6 +- spec/system/budget_polls/voter_spec.rb | 10 ++-- spec/system/budgets/ballots_spec.rb | 30 +++++----- 5 files changed, 66 insertions(+), 61 deletions(-) diff --git a/app/components/budgets/investments/ballot_component.html.erb b/app/components/budgets/investments/ballot_component.html.erb index 3866e5098..21683337c 100644 --- a/app/components/budgets/investments/ballot_component.html.erb +++ b/app/components/budgets/investments/ballot_component.html.erb @@ -8,14 +8,15 @@ <%= investment.formatted_price %>

<% if investment.should_show_ballots? %> - <%= link_to t("budgets.ballots.show.remove"), - budget_ballot_line_path(id: investment.id, - budget_id: investment.budget_id, - investments_ids: investment_ids), - class: "button button-remove-support expanded", - method: :delete, - remote: true, - "aria-label": remove_vote_aria_label %> + <%= button_to budget_ballot_line_path(id: investment.id, + budget_id: investment.budget_id, + investments_ids: investment_ids), + class: "button button-remove-support expanded", + method: :delete, + remote: true, + "aria-label": remove_vote_aria_label do %> + <%= t("budgets.ballots.show.remove") %> + <% end %> <% end %> <% else %> @@ -24,30 +25,34 @@ <%= investment.formatted_price %>

<% if investment.should_show_ballots? %> - <%= link_to t("budgets.investments.investment.add"), - budget_ballot_lines_path(investment_id: investment.id, - budget_id: investment.budget_id, - investments_ids: investment_ids), - class: "button button-support expanded", - title: t("budgets.investments.investment.support_title"), - method: :post, - remote: true, - "aria-label": vote_aria_label %> + <%= button_to budget_ballot_lines_path(investment_id: investment.id, + budget_id: investment.budget_id, + investments_ids: investment_ids), + class: "button button-support expanded", + title: t("budgets.investments.investment.support_title"), + method: :post, + remote: true, + disabled: reason.present?, + "aria-label": vote_aria_label do %> + <%= t("budgets.investments.investment.add") %> + <% end %> <% end %> <% end %> <% if reason.present? && !voted? %> - diff --git a/spec/components/budgets/investments/ballot_component_spec.rb b/spec/components/budgets/investments/ballot_component_spec.rb index 0048246c7..00aee0304 100644 --- a/spec/components/budgets/investments/ballot_component_spec.rb +++ b/spec/components/budgets/investments/ballot_component_spec.rb @@ -1,7 +1,7 @@ require "rails_helper" describe Budgets::Investments::BallotComponent do - describe "vote investment link" do + describe "vote investment button" do let(:budget) { create(:budget, :balloting) } let(:investment) { create(:budget_investment, :selected, title: "New Sports Center", budget: budget) } let(:component) do @@ -18,7 +18,7 @@ describe Budgets::Investments::BallotComponent do render_inline component - expect(page).to have_link "Vote" + expect(page).to have_button "Vote", disabled: true expect(page).to have_content "Only verified users can vote on investments; verify your account." end @@ -27,21 +27,21 @@ describe Budgets::Investments::BallotComponent do render_inline component - expect(page).to have_link count: 1 - expect(page).to have_link "Vote", title: "Support this project" - expect(page).to have_link "Vote New Sports Center" - expect(page).not_to have_link "Remove vote" + expect(page).to have_button count: 1 + expect(page).to have_button "Vote", title: "Support this project" + expect(page).to have_button "Vote New Sports Center" + expect(page).not_to have_button "Remove vote", disabled: :all end - it "is replaced with a link to remove the vote when the user has already voted" do + it "is replaced with a button to remove the vote when the user has already voted" do sign_in(create(:user, :level_two, ballot_lines: [investment])) render_inline component - expect(page).to have_link count: 1 - expect(page).to have_link "Remove vote" - expect(page).to have_link "Remove your vote for New Sports Center" - expect(page).not_to have_link "Vote" + expect(page).to have_button count: 1 + expect(page).to have_button "Remove vote" + expect(page).to have_button "Remove your vote for New Sports Center" + expect(page).not_to have_button "Vote", disabled: :all end end end diff --git a/spec/support/common_actions/budgets.rb b/spec/support/common_actions/budgets.rb index baf7b058c..f36e143c3 100644 --- a/spec/support/common_actions/budgets.rb +++ b/spec/support/common_actions/budgets.rb @@ -1,17 +1,17 @@ module Budgets def expect_message_organizations_cannot_vote expect(page).to have_content "Organization" - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end def hover_over_ballot scroll_to find("div.ballot"), align: :bottom - find("div.ballot").hover + first("div.ballot p").hover end def add_to_ballot(investment_title) within(".budget-investment", text: investment_title) do - find(".add a").click + click_button "Vote" expect(page).to have_content "Remove" end end diff --git a/spec/system/budget_polls/voter_spec.rb b/spec/system/budget_polls/voter_spec.rb index 243fe1b10..66b457c27 100644 --- a/spec/system/budget_polls/voter_spec.rb +++ b/spec/system/budget_polls/voter_spec.rb @@ -81,11 +81,11 @@ describe "BudgetPolls", :with_frozen_time do login_as(user) visit budget_investment_path(budget, investment) - find("div.ballot").hover + first("div.ballot p").hover within("#budget_investment_#{investment.id}") do expect(page).to have_content "You have already participated offline" - expect(page).to have_css(".add a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end end end @@ -96,7 +96,7 @@ describe "BudgetPolls", :with_frozen_time do visit budget_investment_path(budget, investment) within("#budget_investment_#{investment.id}") do - find(".add a").click + click_button "Vote" expect(page).to have_content "Remove" end end @@ -106,7 +106,7 @@ describe "BudgetPolls", :with_frozen_time do visit budget_investment_path(budget, investment) within("#budget_investment_#{investment.id}") do - find(".add a").click + click_button "Vote" expect(page).to have_content "Remove" end @@ -123,7 +123,7 @@ describe "BudgetPolls", :with_frozen_time do visit budget_investment_path(budget, investment) within("#budget_investment_#{investment.id}") do - find(".add a").click + click_button "Vote" expect(page).to have_content "Remove" end diff --git a/spec/system/budgets/ballots_spec.rb b/spec/system/budgets/ballots_spec.rb index 8a253b613..e95075a5e 100644 --- a/spec/system/budgets/ballots_spec.rb +++ b/spec/system/budgets/ballots_spec.rb @@ -147,7 +147,7 @@ describe "Ballots" do end within("#budget_investment_#{investment.id}") do - find(".remove a").click + click_button "Remove vote" end expect(page).to have_css("#total_amount", text: "€0") @@ -283,8 +283,8 @@ describe "Ballots" do visit budget_investments_path(budget, heading_id: california.id) within(".budget-investment", text: "Early ShakeAlert") do - find(".remove a").click - expect(page).to have_link "Vote" + click_button "Remove vote" + expect(page).to have_button "Vote" end visit budget_investments_path(budget, heading_id: new_york.id) @@ -449,7 +449,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_content "You must sign in or sign up to continue." - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end end @@ -515,7 +515,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_content("already voted a different heading") - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end end @@ -531,7 +531,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end end @@ -546,7 +546,7 @@ describe "Ballots" do hover_over_ballot expect(page).not_to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: false) + expect(page).to have_button "Vote", obscured: false end add_to_ballot("Build replicants") @@ -555,7 +555,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end end @@ -571,7 +571,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end within("#budget_investment_#{bi1.id}") do @@ -583,7 +583,7 @@ describe "Ballots" do hover_over_ballot expect(page).not_to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: false) + expect(page).to have_button "Vote", obscured: false end end @@ -599,7 +599,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end within("#budget_investment_#{bi1.id}_sidebar") do @@ -612,7 +612,7 @@ describe "Ballots" do hover_over_ballot expect(page).not_to have_content("You have already assigned the available budget") - expect(page).to have_selector(".in-favor a", obscured: false) + expect(page).to have_button "Vote", obscured: false end end @@ -623,7 +623,7 @@ describe "Ballots" do login_as user visit budget_investments_path(budget, heading_id: new_york) - expect(page).to have_css(".in-favor a") + expect(page).to have_button "Vote" in_browser(:admin) do login_as admin_user @@ -639,7 +639,7 @@ describe "Ballots" do end within("#budget_investment_#{investment1.id}") do - find(".in-favor a").click + click_button "Vote" expect(page).not_to have_content "Remove" expect(page).not_to have_selector(".participation-not-allowed") @@ -647,7 +647,7 @@ describe "Ballots" do hover_over_ballot expect(page).to have_selector(".participation-not-allowed") - expect(page).to have_selector(".in-favor a", obscured: true) + expect(page).to have_button "Vote", disabled: true, obscured: true end end