From ccbc708b5b2bcacfaf11e1ebc9f0159fe2628edb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Fri, 25 Jun 2021 22:32:11 +0200
Subject: [PATCH 1/3] Simplify poll select options helper

The `include_all` parameter was always used, and the option was
redundant because we already had a prompt offering the same
functionality.

I guess one possible reason was users would want to filter by all polls,
and having to click on "select a poll" to do so wasn't that intuitive.
So we're using "All" as the prompt instead.
---
 app/helpers/polls_helper.rb                     | 7 +------
 app/views/admin/poll/questions/_filter.html.erb | 4 ++--
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/app/helpers/polls_helper.rb b/app/helpers/polls_helper.rb
index b69a9368f..bf666d047 100644
--- a/app/helpers/polls_helper.rb
+++ b/app/helpers/polls_helper.rb
@@ -1,16 +1,11 @@
 module PollsHelper
-  def poll_select_options(include_all = nil)
+  def poll_select_options
     options = @polls.map do |poll|
       [poll.name, current_path_with_query_params(poll_id: poll.id)]
     end
-    options << all_polls if include_all
     options_for_select(options, request.fullpath)
   end
 
-  def all_polls
-    [I18n.t("polls.all"), admin_questions_path]
-  end
-
   def poll_dates(poll)
     if poll.starts_at.blank? || poll.ends_at.blank?
       I18n.t("polls.no_dates")
diff --git a/app/views/admin/poll/questions/_filter.html.erb b/app/views/admin/poll/questions/_filter.html.erb
index b3fca61c5..b1e988d8d 100644
--- a/app/views/admin/poll/questions/_filter.html.erb
+++ b/app/views/admin/poll/questions/_filter.html.erb
@@ -1,7 +1,7 @@
 <%= form_tag "", method: :get do %>
   <%= label_tag :poll_id, t("admin.questions.index.filter_poll") %>
   <%= select_tag "poll_id",
-                 poll_select_options(true),
-                 prompt: t("admin.questions.index.select_poll"),
+                 poll_select_options,
+                 prompt: t("polls.all"),
                  class: "js-location-changer" %>
 <% end %>

From 422404085f276a8c08feb75653675f020bd80222 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Mon, 28 Jun 2021 04:26:03 +0200
Subject: [PATCH 2/3] Extract poll questions filter to a component

---
 .../poll/questions/filter_component.html.erb}   |  0
 .../admin/poll/questions/filter_component.rb    | 17 +++++++++++++++++
 app/helpers/polls_helper.rb                     |  7 -------
 .../admin/poll/questions/_questions.html.erb    |  2 +-
 4 files changed, 18 insertions(+), 8 deletions(-)
 rename app/{views/admin/poll/questions/_filter.html.erb => components/admin/poll/questions/filter_component.html.erb} (100%)
 create mode 100644 app/components/admin/poll/questions/filter_component.rb

diff --git a/app/views/admin/poll/questions/_filter.html.erb b/app/components/admin/poll/questions/filter_component.html.erb
similarity index 100%
rename from app/views/admin/poll/questions/_filter.html.erb
rename to app/components/admin/poll/questions/filter_component.html.erb
diff --git a/app/components/admin/poll/questions/filter_component.rb b/app/components/admin/poll/questions/filter_component.rb
new file mode 100644
index 000000000..881a8383b
--- /dev/null
+++ b/app/components/admin/poll/questions/filter_component.rb
@@ -0,0 +1,17 @@
+class Admin::Poll::Questions::FilterComponent < ApplicationComponent
+  attr_reader :polls
+  delegate :current_path_with_query_params, to: :helpers
+
+  def initialize(polls)
+    @polls = polls
+  end
+
+  private
+
+    def poll_select_options
+      options = polls.map do |poll|
+        [poll.name, current_path_with_query_params(poll_id: poll.id)]
+      end
+      options_for_select(options, request.fullpath)
+    end
+end
diff --git a/app/helpers/polls_helper.rb b/app/helpers/polls_helper.rb
index bf666d047..08a2d3ab2 100644
--- a/app/helpers/polls_helper.rb
+++ b/app/helpers/polls_helper.rb
@@ -1,11 +1,4 @@
 module PollsHelper
-  def poll_select_options
-    options = @polls.map do |poll|
-      [poll.name, current_path_with_query_params(poll_id: poll.id)]
-    end
-    options_for_select(options, request.fullpath)
-  end
-
   def poll_dates(poll)
     if poll.starts_at.blank? || poll.ends_at.blank?
       I18n.t("polls.no_dates")
diff --git a/app/views/admin/poll/questions/_questions.html.erb b/app/views/admin/poll/questions/_questions.html.erb
index d83428879..4550796d5 100644
--- a/app/views/admin/poll/questions/_questions.html.erb
+++ b/app/views/admin/poll/questions/_questions.html.erb
@@ -1,5 +1,5 @@
 <div class="small-12 medium-4 large-3">
-  <%= render "filter" %>
+  <%= render Admin::Poll::Questions::FilterComponent.new(@polls) %>
 </div>
 
 <% if @questions.count == 0 %>

From e4f8f702c72fd2dbc09267a9d722f549bfae6465 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Mon, 28 Jun 2021 04:26:51 +0200
Subject: [PATCH 3/3] Use a submit button in admin poll question filters

As mentioned in commit 5214d89c8, using a `<select>` tag which
automatically submits a form on change has a few accessibility issues,
particularly for keyboard users who might accidentally submit the form
while browsing the options.

So we're adding a submit button and removing the "submit on change"
behavior.

Note that, while `<select>` tags have their own usability issues,
alternatives in this case are not obvious because the number of existing
polls could be very low (zero, for instance) or very high (dozens, if
the application has been used for years).

I thought of using a `<datalist>` tag with a regular text input. The
problem here is we don't want to send the name of the poll to the server
(as we would with a `<datalist>` tag); we want to send the ID of the
poll.

Maybe we could add an automplete field instead, providing a similar
funcionality. However, for now we're keeping it simple. This poll
questions page isn't even accessible through the admin menu since commit
83e8d603, so right now anything we change here will be pretty much
useless.
---
 .../stylesheets/admin/poll/questions/filter.scss | 16 ++++++++++++++++
 .../poll/questions/filter_component.html.erb     | 12 ++++++------
 .../admin/poll/questions/filter_component.rb     |  5 +----
 .../admin/poll/questions/_questions.html.erb     |  4 +---
 .../poll/questions/filter_component_spec.rb      |  9 +++++++++
 5 files changed, 33 insertions(+), 13 deletions(-)
 create mode 100644 app/assets/stylesheets/admin/poll/questions/filter.scss
 create mode 100644 spec/components/admin/poll/questions/filter_component_spec.rb

diff --git a/app/assets/stylesheets/admin/poll/questions/filter.scss b/app/assets/stylesheets/admin/poll/questions/filter.scss
new file mode 100644
index 000000000..78cb4fb4f
--- /dev/null
+++ b/app/assets/stylesheets/admin/poll/questions/filter.scss
@@ -0,0 +1,16 @@
+.admin .poll-questions-filter {
+  $gap: 0.5em;
+  align-items: flex-end;
+  display: flex;
+  flex-wrap: wrap;
+  margin-left: -$gap;
+
+  > * {
+    margin-left: $gap;
+  }
+
+  [type="submit"] {
+    @include regular-button;
+    margin-left: $gap;
+  }
+}
diff --git a/app/components/admin/poll/questions/filter_component.html.erb b/app/components/admin/poll/questions/filter_component.html.erb
index b1e988d8d..ee1f443ff 100644
--- a/app/components/admin/poll/questions/filter_component.html.erb
+++ b/app/components/admin/poll/questions/filter_component.html.erb
@@ -1,7 +1,7 @@
-<%= form_tag "", method: :get do %>
-  <%= label_tag :poll_id, t("admin.questions.index.filter_poll") %>
-  <%= select_tag "poll_id",
-                 poll_select_options,
-                 prompt: t("polls.all"),
-                 class: "js-location-changer" %>
+<%= form_tag "", method: :get, class: "poll-questions-filter" do %>
+  <div class="filter">
+    <%= label_tag :poll_id, t("admin.questions.index.filter_poll") %>
+    <%= select_tag "poll_id", poll_select_options, prompt: t("polls.all") %>
+  </div>
+  <%= submit_tag t("shared.filter") %>
 <% end %>
diff --git a/app/components/admin/poll/questions/filter_component.rb b/app/components/admin/poll/questions/filter_component.rb
index 881a8383b..b8b7d7412 100644
--- a/app/components/admin/poll/questions/filter_component.rb
+++ b/app/components/admin/poll/questions/filter_component.rb
@@ -9,9 +9,6 @@ class Admin::Poll::Questions::FilterComponent < ApplicationComponent
   private
 
     def poll_select_options
-      options = polls.map do |poll|
-        [poll.name, current_path_with_query_params(poll_id: poll.id)]
-      end
-      options_for_select(options, request.fullpath)
+      options_from_collection_for_select(polls, :id, :name, params[:poll_id])
     end
 end
diff --git a/app/views/admin/poll/questions/_questions.html.erb b/app/views/admin/poll/questions/_questions.html.erb
index 4550796d5..e8ce0402e 100644
--- a/app/views/admin/poll/questions/_questions.html.erb
+++ b/app/views/admin/poll/questions/_questions.html.erb
@@ -1,6 +1,4 @@
-<div class="small-12 medium-4 large-3">
-  <%= render Admin::Poll::Questions::FilterComponent.new(@polls) %>
-</div>
+<%= render Admin::Poll::Questions::FilterComponent.new(@polls) %>
 
 <% if @questions.count == 0 %>
   <div class="callout primary margin-top">
diff --git a/spec/components/admin/poll/questions/filter_component_spec.rb b/spec/components/admin/poll/questions/filter_component_spec.rb
new file mode 100644
index 000000000..8861ad354
--- /dev/null
+++ b/spec/components/admin/poll/questions/filter_component_spec.rb
@@ -0,0 +1,9 @@
+require "rails_helper"
+
+describe Admin::Poll::Questions::FilterComponent, type: :component do
+  it "renders a button to submit the form" do
+    render_inline Admin::Poll::Questions::FilterComponent.new([])
+
+    expect(page).to have_button "Filter"
+  end
+end