From e4ad70fd822e500fa02309e1eee982feb973cca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 12 Jun 2021 03:09:21 +0200 Subject: [PATCH 01/11] Revert "Adds .js-class for specs" The `js-` prefix (which I admit I'm not fond of) is usually used to indicate the class is used by JavaScript files, not for using it in test files. And in all the other similar tests, we're using the `in-favor` class instead of the `js-in-favor` class. This reverts commit 83fe74d53. --- app/views/budgets/investments/_votes.html.erb | 2 +- spec/system/management/budget_investments_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/views/budgets/investments/_votes.html.erb b/app/views/budgets/investments/_votes.html.erb index e520ec3cc..c2924b990 100644 --- a/app/views/budgets/investments/_votes.html.erb +++ b/app/views/budgets/investments/_votes.html.erb @@ -8,7 +8,7 @@ <%= t("budgets.investments.investment.supports", count: investment.total_votes) %> -
+
<% if user_voted_for %>
<%= t("budgets.investments.investment.already_supported") %> diff --git a/spec/system/management/budget_investments_spec.rb b/spec/system/management/budget_investments_spec.rb index f9273a49c..82fb60c6f 100644 --- a/spec/system/management/budget_investments_spec.rb +++ b/spec/system/management/budget_investments_spec.rb @@ -277,7 +277,7 @@ describe "Budget Investments" do expect(page).to have_content(budget_investment.title) within("#budget-investments") do - find(".js-in-favor a").click + find("in-favor a").click expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" @@ -302,7 +302,7 @@ describe "Budget Investments" do expect(page).to have_css "h1", exact_text: budget_investment.title - find(".js-in-favor a").click + find(".in-favor a").click expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" @@ -325,7 +325,7 @@ describe "Budget Investments" do expect(page).to have_css "h1", exact_text: "Default heading investment" - accept_confirm { find(".js-in-favor a").click } + accept_confirm { find(".in-favor a").click } expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" From 57628a78d9e7736426525ff09f433a72f88da259 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 12 Jun 2021 02:40:06 +0200 Subject: [PATCH 02/11] Move investment votes partial to a component This way changing it will be easier. --- .../investments/votes_component.html.erb | 56 +++++++++++++++++ .../budgets/investments/votes_component.rb | 23 +++++++ app/helpers/accessibility_helper.rb | 5 -- app/helpers/budgets_helper.rb | 6 -- app/views/budgets/investments/_votes.html.erb | 61 ++----------------- 5 files changed, 84 insertions(+), 67 deletions(-) create mode 100644 app/components/budgets/investments/votes_component.html.erb create mode 100644 app/components/budgets/investments/votes_component.rb delete mode 100644 app/helpers/accessibility_helper.rb diff --git a/app/components/budgets/investments/votes_component.html.erb b/app/components/budgets/investments/votes_component.html.erb new file mode 100644 index 000000000..efe3e63cc --- /dev/null +++ b/app/components/budgets/investments/votes_component.html.erb @@ -0,0 +1,56 @@ +<% reason = investment.reason_for_not_being_selectable_by(current_user) %> +<% voting_allowed = true unless reason.presence == :not_voting_allowed %> +<% user_voted_for = voted_for?(investment_votes, investment) %> + +
+ + "> + <%= t("budgets.investments.investment.supports", count: investment.total_votes) %> + + +
+ <% if user_voted_for %> +
+ <%= t("budgets.investments.investment.already_supported") %> +
+ <% elsif investment.should_show_votes? %> + <%= link_to vote_url, + class: "button button-support small expanded", + title: t("budgets.investments.investment.support_title"), + method: "post", + remote: (display_support_alert? ? false : true), + data: (display_support_alert? ? { + confirm: t("budgets.investments.investment.confirm_group", count: investment.group.max_votable_headings) } : nil), + "aria-hidden" => css_for_aria_hidden(reason) do %> + <%= t("budgets.investments.investment.give_support") %> + <% end %> + <% end %> +
+ + <% if reason.present? && !user_voted_for %> + + <% end %> + + <% if user_voted_for && setting["twitter_handle"] %> + + <% end %> +
diff --git a/app/components/budgets/investments/votes_component.rb b/app/components/budgets/investments/votes_component.rb new file mode 100644 index 000000000..c8c9a12d3 --- /dev/null +++ b/app/components/budgets/investments/votes_component.rb @@ -0,0 +1,23 @@ +class Budgets::Investments::VotesComponent < ApplicationComponent + attr_reader :investment, :investment_votes, :vote_url + delegate :current_user, :voted_for?, :image_absolute_url, + :link_to_verify_account, :link_to_signin, :link_to_signup, to: :helpers + + def initialize(investment, investment_votes:, vote_url:) + @investment = investment + @investment_votes = investment_votes + @vote_url = vote_url + end + + private + + def display_support_alert? + current_user && + !current_user.voted_in_group?(investment.group) && + investment.group.headings.count > 1 + end + + def css_for_aria_hidden(reason) + reason.present? ? "true" : "" + end +end diff --git a/app/helpers/accessibility_helper.rb b/app/helpers/accessibility_helper.rb deleted file mode 100644 index b53ada9ad..000000000 --- a/app/helpers/accessibility_helper.rb +++ /dev/null @@ -1,5 +0,0 @@ -module AccessibilityHelper - def css_for_aria_hidden(reason) - reason.present? ? "true" : "" - end -end diff --git a/app/helpers/budgets_helper.rb b/app/helpers/budgets_helper.rb index e07b73192..f7c196bbb 100644 --- a/app/helpers/budgets_helper.rb +++ b/app/helpers/budgets_helper.rb @@ -54,12 +54,6 @@ module BudgetsHelper end end - def display_support_alert?(investment) - current_user && - !current_user.voted_in_group?(investment.group) && - investment.group.headings.count > 1 - end - def budget_subnav_items_for(budget) { results: t("budgets.results.link"), diff --git a/app/views/budgets/investments/_votes.html.erb b/app/views/budgets/investments/_votes.html.erb index c2924b990..d134506c1 100644 --- a/app/views/budgets/investments/_votes.html.erb +++ b/app/views/budgets/investments/_votes.html.erb @@ -1,56 +1,5 @@ -<% reason = investment.reason_for_not_being_selectable_by(current_user) %> -<% voting_allowed = true unless reason.presence == :not_voting_allowed %> -<% user_voted_for = voted_for?(investment_votes, investment) %> - -
- - "> - <%= t("budgets.investments.investment.supports", count: investment.total_votes) %> - - -
- <% if user_voted_for %> -
- <%= t("budgets.investments.investment.already_supported") %> -
- <% elsif investment.should_show_votes? %> - <%= link_to vote_url, - class: "button button-support small expanded", - title: t("budgets.investments.investment.support_title"), - method: "post", - remote: (display_support_alert?(investment) ? false : true), - data: (display_support_alert?(investment) ? { - confirm: t("budgets.investments.investment.confirm_group", count: investment.group.max_votable_headings) } : nil), - "aria-hidden" => css_for_aria_hidden(reason) do %> - <%= t("budgets.investments.investment.give_support") %> - <% end %> - <% end %> -
- - <% if reason.present? && !user_voted_for %> - - <% end %> - - <% if user_voted_for && setting["twitter_handle"] %> - - <% end %> -
+<%= render Budgets::Investments::VotesComponent.new( + investment, + investment_votes: investment_votes, + vote_url: vote_url +) %> From ad6dd54da2ffee8829270c0d540c52da89e49408 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 12 Jun 2021 02:47:36 +0200 Subject: [PATCH 03/11] Simplify rule to display support alert --- app/components/budgets/investments/votes_component.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/budgets/investments/votes_component.html.erb b/app/components/budgets/investments/votes_component.html.erb index efe3e63cc..5a38de879 100644 --- a/app/components/budgets/investments/votes_component.html.erb +++ b/app/components/budgets/investments/votes_component.html.erb @@ -18,7 +18,7 @@ class: "button button-support small expanded", title: t("budgets.investments.investment.support_title"), method: "post", - remote: (display_support_alert? ? false : true), + remote: !display_support_alert?, data: (display_support_alert? ? { confirm: t("budgets.investments.investment.confirm_group", count: investment.group.max_votable_headings) } : nil), "aria-hidden" => css_for_aria_hidden(reason) do %> From cd8fa606c7571d77dc5ace97706aa3d94d9f515f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 12 Jun 2021 02:49:00 +0200 Subject: [PATCH 04/11] Use methods instead of variables in votes view Now the view is more readable and it's possible to customize these methods writing a custom class, without changing or copying the view. --- .../budgets/investments/votes_component.html.erb | 14 +++++--------- .../budgets/investments/votes_component.rb | 14 +++++++++++++- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/app/components/budgets/investments/votes_component.html.erb b/app/components/budgets/investments/votes_component.html.erb index 5a38de879..6087f6820 100644 --- a/app/components/budgets/investments/votes_component.html.erb +++ b/app/components/budgets/investments/votes_component.html.erb @@ -1,15 +1,11 @@ -<% reason = investment.reason_for_not_being_selectable_by(current_user) %> -<% voting_allowed = true unless reason.presence == :not_voting_allowed %> -<% user_voted_for = voted_for?(investment_votes, investment) %> -
- "> + "> <%= t("budgets.investments.investment.supports", count: investment.total_votes) %>
- <% if user_voted_for %> + <% if user_voted_for? %>
<%= t("budgets.investments.investment.already_supported") %>
@@ -21,13 +17,13 @@ remote: !display_support_alert?, data: (display_support_alert? ? { confirm: t("budgets.investments.investment.confirm_group", count: investment.group.max_votable_headings) } : nil), - "aria-hidden" => css_for_aria_hidden(reason) do %> + "aria-hidden" => css_for_aria_hidden do %> <%= t("budgets.investments.investment.give_support") %> <% end %> <% end %>
- <% if reason.present? && !user_voted_for %> + <% if reason.present? && !user_voted_for? %> <% end %> - <% if user_voted_for && setting["twitter_handle"] %> + <% if user_voted_for? && setting["twitter_handle"] %> <% elsif investment.should_show_votes? %> - <%= link_to vote_url, + <%= link_to t("budgets.investments.investment.give_support"), vote_url, class: "button button-support small expanded", title: t("budgets.investments.investment.support_title"), method: "post", remote: !display_support_alert?, data: ({ confirm: confirm_vote_message } if display_support_alert?), - "aria-hidden" => css_for_aria_hidden do %> - <%= t("budgets.investments.investment.give_support") %> - <% end %> + "aria-hidden" => css_for_aria_hidden %> <% end %>
From da11c0d7bab9fb54c8b5cee8a9e1d0ed13b41ca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sat, 12 Jun 2021 14:34:27 +0200 Subject: [PATCH 07/11] Fix empty support link for screen readers users When identified users accessed the investments page, we were using the `aria-hidden` attribute to hide this link from screen readers since unidentified users can't support investments. However, the link was still focusable using keyboard navigation. This resulted in screen reader users reaching the link and being able to click it, but getting no feedback at all about where they were or what they were clicking on. This is why the fourth ARIA rule says: "Do not use role="presentation" or aria-hidden="true" on a focusable element" [1]. So we're replacing the link with a non-interactive element instead, like we do in other places like proposals. The accessibility of this part of the page for unidentified users still has some issues; here we're only improving it up to a certain point. [1] https://www.w3.org/TR/using-aria/#4thrule --- .../investments/votes_component.html.erb | 45 +++++++++++-------- .../budgets/investments/votes_component.rb | 4 -- .../investments/votes_component_spec.rb | 31 +++++++++++++ 3 files changed, 57 insertions(+), 23 deletions(-) create mode 100644 spec/components/budgets/investments/votes_component_spec.rb diff --git a/app/components/budgets/investments/votes_component.html.erb b/app/components/budgets/investments/votes_component.html.erb index 8b84af637..70ffbfe96 100644 --- a/app/components/budgets/investments/votes_component.html.erb +++ b/app/components/budgets/investments/votes_component.html.erb @@ -10,29 +10,36 @@ <%= t("budgets.investments.investment.already_supported") %>
<% elsif investment.should_show_votes? %> - <%= link_to t("budgets.investments.investment.give_support"), vote_url, - class: "button button-support small expanded", - title: t("budgets.investments.investment.support_title"), - method: "post", - remote: !display_support_alert?, - data: ({ confirm: confirm_vote_message } if display_support_alert?), - "aria-hidden" => css_for_aria_hidden %> + <% if current_user %> + <%= link_to t("budgets.investments.investment.give_support"), vote_url, + class: "button button-support small expanded", + title: t("budgets.investments.investment.support_title"), + method: "post", + remote: !display_support_alert?, + data: ({ confirm: confirm_vote_message } if display_support_alert?) %> + <% else %> +
+ <%= t("budgets.investments.investment.give_support") %> +
+ <% end %> <% end %>
<% if reason.present? && !user_voted_for? %> - diff --git a/spec/components/budgets/investments/votes_component_spec.rb b/spec/components/budgets/investments/votes_component_spec.rb index 888d41ef5..0b85f7f20 100644 --- a/spec/components/budgets/investments/votes_component_spec.rb +++ b/spec/components/budgets/investments/votes_component_spec.rb @@ -10,23 +10,22 @@ describe Budgets::Investments::VotesComponent, type: :component do before { allow(investment).to receive(:should_show_votes?).and_return(true) } - it "displays a link to support the investment to identified users" do + it "displays a button to support the investment to identified users" do allow(controller).to receive(:current_user).and_return(create(:user)) render_inline component - expect(page).to have_link count: 1 - expect(page).to have_link "Support", title: "Support this project" - expect(page).to have_link "Support Renovate sidewalks in Main Street" + expect(page).to have_button count: 1 + expect(page).to have_button "Support", title: "Support this project", disabled: false + expect(page).to have_button "Support Renovate sidewalks in Main Street" end - it "does not display link to support the investment to unidentified users" do + it "disables the button to support the investment to unidentified users" do allow(controller).to receive(:current_user).and_return(nil) render_inline component - expect(page).not_to have_link "Support" - expect(page).to have_content "Support" + expect(page).to have_button "Support", disabled: true end end end diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb index af1261847..2b1d3ae48 100644 --- a/spec/system/budgets/investments_spec.rb +++ b/spec/system/budgets/investments_spec.rb @@ -1113,7 +1113,9 @@ describe "Budget Investments" do visit budget_investments_path(budget, heading_id: carabanchel.id) within(".budget-investment", text: "In Carabanchel") do - expect(page).to have_css(".in-favor a[data-confirm]") + expect(page).to have_button count: 1 + expect(page).to have_button "Support" + expect(page).to have_css "[type='submit'][data-confirm]" end end @@ -1129,8 +1131,8 @@ describe "Budget Investments" do visit budget_investments_path(budget, heading_id: carabanchel.id) within(".budget-investment", text: "Unsupported in Carabanchel") do - expect(page).to have_link "Support" - expect(page).not_to have_css(".in-favor a[data-confirm]") + expect(page).to have_button "Support" + expect(page).not_to have_css "[data-confirm]" end end @@ -1148,7 +1150,9 @@ describe "Budget Investments" do visit budget_investments_path(budget, heading_id: another_heading1.id) within(".budget-investment", text: "Another investment") do - expect(page).to have_css(".in-favor a[data-confirm]") + expect(page).to have_button count: 1 + expect(page).to have_button "Support" + expect(page).to have_css "[type='submit'][data-confirm]" end end @@ -1159,7 +1163,8 @@ describe "Budget Investments" do visit budget_investments_path(budget, heading_id: heading.id) within("#budget_investment_#{all_city_investment.id}") do - expect(page).not_to have_css(".in-favor a[data-confirm]") + expect(page).to have_button "Support" + expect(page).not_to have_css "[data-confirm]" end end end diff --git a/spec/system/budgets/votes_spec.rb b/spec/system/budgets/votes_spec.rb index 8c5e64137..5e1aff243 100644 --- a/spec/system/budgets/votes_spec.rb +++ b/spec/system/budgets/votes_spec.rb @@ -42,7 +42,7 @@ describe "Votes" do visit budget_investments_path(budget, heading_id: heading.id) within(".supports") do - click_link "Support" + click_button "Support" expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. "\ @@ -63,10 +63,10 @@ describe "Votes" do visit budget_investment_path(budget, investment) within(".supports") do - find(".in-favor a").click - expect(page).to have_content "1 support" + click_button "Support" - expect(page).not_to have_selector ".in-favor a" + expect(page).not_to have_button "Support", disabled: :all + expect(page).to have_content "1 support" end end @@ -74,7 +74,7 @@ describe "Votes" do visit budget_investment_path(budget, investment) within(".supports") do - find(".in-favor a").click + click_button "Support" expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. "\ @@ -120,7 +120,7 @@ describe "Votes" do visit budget_investments_path(budget, heading_id: new_york.id) within("#budget_investment_#{new_york_investment.id}") do - accept_confirm { find(".in-favor a").click } + accept_confirm { click_button "Support" } expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. "\ @@ -130,7 +130,7 @@ describe "Votes" do visit budget_investments_path(budget, heading_id: san_francisco.id) within("#budget_investment_#{san_francisco_investment.id}") do - find(".in-favor a").click + click_button "Support" expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. "\ @@ -140,7 +140,7 @@ describe "Votes" do visit budget_investments_path(budget, heading_id: third_heading.id) within("#budget_investment_#{third_heading_investment.id}") do - find(".in-favor a").click + click_button "Support" expect(page).to have_content "You can only support investment projects in 2 districts. "\ "You have already supported investments in" @@ -160,19 +160,22 @@ describe "Votes" do scenario "From show" do visit budget_investment_path(budget, new_york_investment) - accept_confirm { find(".in-favor a").click } + accept_confirm { click_button "Support" } + expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" visit budget_investment_path(budget, san_francisco_investment) - find(".in-favor a").click + click_button "Support" + expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" visit budget_investment_path(budget, third_heading_investment) - find(".in-favor a").click + click_button "Support" + expect(page).to have_content "You can only support investment projects in 2 districts. "\ "You have already supported investments in" @@ -189,7 +192,7 @@ describe "Votes" do scenario "Confirm message shows the right text" do visit budget_investments_path(budget, heading_id: new_york.id) - find(".in-favor a").click + click_button "Support" expect(page.driver.send(:find_modal).text).to match "You can only support investments in 2 districts." end diff --git a/spec/system/management/budget_investments_spec.rb b/spec/system/management/budget_investments_spec.rb index 82fb60c6f..eafb51d94 100644 --- a/spec/system/management/budget_investments_spec.rb +++ b/spec/system/management/budget_investments_spec.rb @@ -277,7 +277,7 @@ describe "Budget Investments" do expect(page).to have_content(budget_investment.title) within("#budget-investments") do - find("in-favor a").click + click_button "Support" expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" @@ -302,7 +302,7 @@ describe "Budget Investments" do expect(page).to have_css "h1", exact_text: budget_investment.title - find(".in-favor a").click + click_button "Support" expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!" @@ -325,7 +325,7 @@ describe "Budget Investments" do expect(page).to have_css "h1", exact_text: "Default heading investment" - accept_confirm { find(".in-favor a").click } + accept_confirm { click_button "Support" } expect(page).to have_content "1 support" expect(page).to have_content "You have already supported this investment project. Share it!"