From 6dcdf5c84381743a6f4a8ae3b1ceb2ac69c14797 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 28 Jun 2021 22:02:16 +0200 Subject: [PATCH 1/7] Use a button to toggle the advanced search form Users (particularly, screen reader users) usually identify links with things that take you somewhere, and buttons with things that either send forms or change things on the page. Using a button we can also use the `aria-expanded` attribute, meaning screen reader users will know that the button has two states ("expanded" and "collapsed"), the current state of the button, and will get immediate feedback when clicking the button because the new state of the button will be announced. Thanks to this change, we can also slightly simplify the code; we obviously have to remove the (useless) `href` attribute, and we don't have to prevent the default event in JavaScript since there's no default event for buttons with `type="button"`. --- app/assets/javascripts/advanced_search.js | 9 ++--- app/assets/stylesheets/advanced_search.scss | 6 +++ .../shared/advanced_search_component.html.erb | 4 +- spec/system/advanced_search_spec.rb | 40 +++++++++---------- 4 files changed, 32 insertions(+), 27 deletions(-) diff --git a/app/assets/javascripts/advanced_search.js b/app/assets/javascripts/advanced_search.js index 9fcf8ae5f..7ca0245a1 100644 --- a/app/assets/javascripts/advanced_search.js +++ b/app/assets/javascripts/advanced_search.js @@ -4,10 +4,6 @@ advanced_search_terms: function() { return $("#js-advanced-search").data("advanced-search-terms"); }, - toggle_form: function(event) { - event.preventDefault(); - $("#js-advanced-search").slideToggle(); - }, toggle_date_options: function() { if ($("#js-advanced-search-date-min").val() === "custom") { $("#js-custom-date").show(); @@ -23,8 +19,9 @@ App.AdvancedSearch.toggle_date_options(); } $("#js-advanced-search-title").on({ - click: function(event) { - App.AdvancedSearch.toggle_form(event); + click: function() { + $(this).attr("aria-expanded", !JSON.parse($(this).attr("aria-expanded"))); + $("#js-advanced-search").slideToggle(); } }); $("#js-advanced-search-date-min").on({ diff --git a/app/assets/stylesheets/advanced_search.scss b/app/assets/stylesheets/advanced_search.scss index 9191bb83c..2ec042cd0 100644 --- a/app/assets/stylesheets/advanced_search.scss +++ b/app/assets/stylesheets/advanced_search.scss @@ -1,4 +1,6 @@ .advanced-search { + color: $link; + cursor: pointer; float: left; margin: $line-height 0; position: inherit; @@ -10,6 +12,10 @@ position: absolute; right: 0; } + + &:focus { + outline: $outline-focus; + } } .advanced-search-form { diff --git a/app/components/shared/advanced_search_component.html.erb b/app/components/shared/advanced_search_component.html.erb index 903542908..6ac0c3f8b 100644 --- a/app/components/shared/advanced_search_component.html.erb +++ b/app/components/shared/advanced_search_component.html.erb @@ -1,5 +1,7 @@
- <%= link_to t("shared.advanced_search.title"), "#advanced_search_form", id: "js-advanced-search-title", class: "advanced-search small" %> +
diff --git a/spec/system/advanced_search_spec.rb b/spec/system/advanced_search_spec.rb index 8eefdbec3..f6f296663 100644 --- a/spec/system/advanced_search_spec.rb +++ b/spec/system/advanced_search_spec.rb @@ -11,7 +11,7 @@ describe "Advanced search" do visit debates_path - click_link "Advanced search" + click_button "Advanced search" fill_in "Write the text", with: "Schwifty" click_button "Filter" @@ -31,7 +31,7 @@ describe "Advanced search" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" fill_in "Write the text", with: "Schwifty" click_button "Filter" @@ -51,7 +51,7 @@ describe "Advanced search" do visit budget_investments_path(budget) - click_link "Advanced search" + click_button "Advanced search" fill_in "Write the text", with: "Schwifty" click_button "Filter" @@ -76,7 +76,7 @@ describe "Advanced search" do visit debates_path - click_link "Advanced search" + click_button "Advanced search" select "Official position 1", from: "advanced_search_official_level" click_button "Filter" @@ -100,7 +100,7 @@ describe "Advanced search" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" select "Official position 2", from: "advanced_search_official_level" click_button "Filter" @@ -124,7 +124,7 @@ describe "Advanced search" do visit budget_investments_path(budget) - click_link "Advanced search" + click_button "Advanced search" select "Official position 3", from: "advanced_search_official_level" click_button "Filter" @@ -148,7 +148,7 @@ describe "Advanced search" do visit debates_path - click_link "Advanced search" + click_button "Advanced search" select "Official position 4", from: "advanced_search_official_level" click_button "Filter" @@ -172,7 +172,7 @@ describe "Advanced search" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" select "Official position 5", from: "advanced_search_official_level" click_button "Filter" @@ -194,7 +194,7 @@ describe "Advanced search" do visit budget_investments_path(budget) - click_link "Advanced search" + click_button "Advanced search" select "Last 24 hours", from: "js-advanced-search-date-min" click_button "Filter" @@ -214,7 +214,7 @@ describe "Advanced search" do visit debates_path - click_link "Advanced search" + click_button "Advanced search" select "Last week", from: "js-advanced-search-date-min" click_button "Filter" @@ -234,7 +234,7 @@ describe "Advanced search" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" select "Last month", from: "js-advanced-search-date-min" click_button "Filter" @@ -254,7 +254,7 @@ describe "Advanced search" do visit budget_investments_path(budget) - click_link "Advanced search" + click_button "Advanced search" select "Last year", from: "js-advanced-search-date-min" click_button "Filter" @@ -275,7 +275,7 @@ describe "Advanced search" do visit debates_path - click_link "Advanced search" + click_button "Advanced search" select "Customized", from: "js-advanced-search-date-min" fill_in "advanced_search_date_min", with: 7.days.ago.strftime("%d/%m/%Y") fill_in "advanced_search_date_max", with: 1.day.ago.strftime("%d/%m/%Y") @@ -298,7 +298,7 @@ describe "Advanced search" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" select "Customized", from: "js-advanced-search-date-min" fill_in "advanced_search_date_min", with: 4000.years.ago.strftime("%d/%m/%Y") fill_in "advanced_search_date_max", with: "13/13/2199" @@ -325,7 +325,7 @@ describe "Advanced search" do visit budget_investments_path(budget) - click_link "Advanced search" + click_button "Advanced search" fill_in "Write the text", with: "Schwifty" select "Official position 1", from: "advanced_search_official_level" select "Last 24 hours", from: "js-advanced-search-date-min" @@ -343,7 +343,7 @@ describe "Advanced search" do Setting["official_level_1_name"] = "Official position 1" visit debates_path - click_link "Advanced search" + click_button "Advanced search" fill_in "Write the text", with: "Schwifty" select "Official position 1", from: "advanced_search_official_level" @@ -360,7 +360,7 @@ describe "Advanced search" do scenario "Maintain custom date search criteria" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" select "Customized", from: "js-advanced-search-date-min" fill_in "advanced_search_date_min", with: 7.days.ago.strftime("%d/%m/%Y") @@ -391,7 +391,7 @@ describe "Advanced search" do create(:budget_investment, title: "Hospital", heading: heading, sdg_goals: [SDG::Goal[3]]) visit budget_investments_path(budget) - click_link "Advanced search" + click_button "Advanced search" select "6. Clean Water and Sanitation", from: "By SDG" click_button "Filter" @@ -413,7 +413,7 @@ describe "Advanced search" do create(:debate, title: "Preschool", sdg_targets: [SDG::Target["4.2"]]) visit debates_path - click_link "Advanced search" + click_button "Advanced search" select "4.2", from: "By target" click_button "Filter" @@ -429,7 +429,7 @@ describe "Advanced search" do scenario "Dynamic target options depending on the selected goal" do visit proposals_path - click_link "Advanced search" + click_button "Advanced search" select "1. No Poverty", from: "By SDG" expect(page).to have_select "By target", From b0e82d636eb5c84b1cd3a3f2ef39a3147b41a0b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 28 Jun 2021 22:13:31 +0200 Subject: [PATCH 2/7] Remove redundant position property I guess it was there to make a contrast with the CSS used for the version for medium and large screens. However, it isn't necessary and the absence of this property already makes a contrast with the presence of the property for medium and large screens. --- app/assets/stylesheets/advanced_search.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/stylesheets/advanced_search.scss b/app/assets/stylesheets/advanced_search.scss index 2ec042cd0..60640daa8 100644 --- a/app/assets/stylesheets/advanced_search.scss +++ b/app/assets/stylesheets/advanced_search.scss @@ -3,7 +3,6 @@ cursor: pointer; float: left; margin: $line-height 0; - position: inherit; @include breakpoint(medium) { float: right; From 2f5f674b2422fb92a809243bcbfe4cb1d7d1713e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 28 Jun 2021 22:14:34 +0200 Subject: [PATCH 3/7] Fix advanced search button on small screens Once the button was pressed, part of the form appeared next to the button instead of appearing below it. --- app/assets/stylesheets/advanced_search.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/app/assets/stylesheets/advanced_search.scss b/app/assets/stylesheets/advanced_search.scss index 60640daa8..8a4d79c94 100644 --- a/app/assets/stylesheets/advanced_search.scss +++ b/app/assets/stylesheets/advanced_search.scss @@ -1,7 +1,6 @@ .advanced-search { color: $link; cursor: pointer; - float: left; margin: $line-height 0; @include breakpoint(medium) { From f6979d1a797d8d11038c2854132fbc5764664657 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 28 Jun 2021 23:06:59 +0200 Subject: [PATCH 4/7] Simplify advanced search form markup Now the advanced search form is an actual form instead of a div containing a form. --- app/assets/stylesheets/advanced_search.scss | 1 + .../shared/advanced_search_component.html.erb | 126 +++++++++--------- 2 files changed, 62 insertions(+), 65 deletions(-) diff --git a/app/assets/stylesheets/advanced_search.scss b/app/assets/stylesheets/advanced_search.scss index 8a4d79c94..394a0d0b6 100644 --- a/app/assets/stylesheets/advanced_search.scss +++ b/app/assets/stylesheets/advanced_search.scss @@ -17,6 +17,7 @@ } .advanced-search-form { + @include grid-row-nest; @include breakpoint(large) { .filter { diff --git a/app/components/shared/advanced_search_component.html.erb b/app/components/shared/advanced_search_component.html.erb index 6ac0c3f8b..04926b755 100644 --- a/app/components/shared/advanced_search_component.html.erb +++ b/app/components/shared/advanced_search_component.html.erb @@ -4,69 +4,65 @@
-
- <%= form_tag request.path, id: "advanced_search_form", method: :get do %> - +<% end %> From 6a44ca350d8663dc7213039caf73acb2eef2622b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 28 Jun 2021 23:24:13 +0200 Subject: [PATCH 5/7] Render the advanced search form without JavaScript We were using the form and then showing it with JavaScript when advanced search terms were present. Now we hide it with JavaScript when no advanced search are present. This means users without JavaScript (including users with JavaScript enabled but bad internet connections preventing the JavaScript to load) can now access the form. The other main difference between the two versions is the way the form flashes while JavaScript is loading. Previously, the form would always be hidden when no terms had been introduced. However, when these terms were present, after submitting the form it would briefly be hidden and then shown again. Now the opposite happens. When advanced search terms are present, the form is shown at all times. However, when they aren't, the form is briefly shown before it disappears. Here the previous behavior is arguably better because most of the time these terms will not be present. So basically we're significantly improving the experience of some users at the cost of slightly worsen the experience of other users. We're also hiding the button to show the form when JavaScript is disabled, since in this scenario it's useless. We're using the `hidden` attribute so hidden buttons can be detected in CSS. --- app/assets/javascripts/advanced_search.js | 9 +++++++-- .../shared/advanced_search_component.html.erb | 4 ++-- .../shared/advanced_search_component_spec.rb | 18 ++++++++++++++++-- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/advanced_search.js b/app/assets/javascripts/advanced_search.js index 7ca0245a1..89f8be04f 100644 --- a/app/assets/javascripts/advanced_search.js +++ b/app/assets/javascripts/advanced_search.js @@ -14,11 +14,16 @@ } }, initialize: function() { + var toggle_button = $("#js-advanced-search-title"); + + toggle_button.removeAttr("hidden"); + if (App.AdvancedSearch.advanced_search_terms()) { - $("#js-advanced-search").show(); App.AdvancedSearch.toggle_date_options(); + } else { + $("#js-advanced-search").hide(); } - $("#js-advanced-search-title").on({ + toggle_button.on({ click: function() { $(this).attr("aria-expanded", !JSON.parse($(this).attr("aria-expanded"))); $("#js-advanced-search").slideToggle(); diff --git a/app/components/shared/advanced_search_component.html.erb b/app/components/shared/advanced_search_component.html.erb index 04926b755..f221eed99 100644 --- a/app/components/shared/advanced_search_component.html.erb +++ b/app/components/shared/advanced_search_component.html.erb @@ -1,11 +1,11 @@
-
<%= form_tag request.path, id: "advanced_search_form", class: "advanced-search-form", method: :get do %> -