Skip to content

Commit

Permalink
Address code review feedback
Browse files Browse the repository at this point in the history
Specifically:
* Clean up results summary in filter header, as requested by
Jeremy.
* Fix scrollbar overlapping with content, as requested by Jeremy
* The first filter menu in the list should always remain open, as
requested by Darcy.
* CSS changes for applied filters, as requested by Darcy (black, not
bolded, add 'x' icon).
* Move filters container above results. This was not specifically
requested by anyone, but it makes keyboard navigation more reasonable
and will be needed when we add styling for smaller viewports.

Not addressed in this commit:
* Darcy expressed usability concerns about the scroll boxes on
mobile. I tested them on my phone and didn't find them to be
difficult to use, so I'm leaving them in for now. They also seem
to be navigable by keyboard and screen reader.
  * One potential a11y issue is having to navigate through a
  bunch of filter opens before getting to the next category. I can
  discuss this with Darcy in QA, unless the code reviewer has a
  strong opinion.
  * If keyboard nav does feel onerous, an alternative solution is
  to hide the bulk of filter options behind a 'See all' link.
* Darcy has requested that we persist menu state between page loads.
In other words, if a filter drop-down menu is open, it should
remain open until a user manually closes it, even if they have
deselected all filters in that group. I think this is complex
enough to warrant a separate ticket. (I've spent the better part
of today experimenting with Turbolinks and Stimulus, both of which
should be able to achieve this, but I haven't been able to achieve
the desired results.)
  • Loading branch information
jazairi committed Feb 6, 2024
1 parent d2d2106 commit 00608d6
Show file tree
Hide file tree
Showing 5 changed files with 57 additions and 19 deletions.
20 changes: 15 additions & 5 deletions app/assets/stylesheets/partials/_filters.scss
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,12 @@
& + .filter-options {
max-height: 200px;
overflow-y: scroll;
scrollbar-gutter: auto;
scroll-behavior: auto;
}
}
&.expanded:after {
font-family: FontAwesome;
content: '\f078';
}
&::-webkit-details-marker {
Expand Down Expand Up @@ -81,17 +83,25 @@
}

&:hover,
&:focus,
&.applied {
&:focus {
background: #000;
color: #fff;
span {
color: #fff;
}
}

&.applied {
font-weight: $fw-bold;
.name {
color: #000;
&:hover,
&:focus {
color: #fff;
}
}
.name::after {
font-family: FontAwesome;
margin-left: 0.5rem;
content: '\f00d';
}
}
}
}
Expand Down
5 changes: 5 additions & 0 deletions app/helpers/results_helper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module ResultsHelper
def results_summary(hits)
hits.to_i >= 10000 ? '10,000+ items' : "#{number_with_delimiter(hits)} items"
end
end
2 changes: 1 addition & 1 deletion app/views/search/_filter.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<% return if values.empty? %>

<div class="category">
<button class="filter-label <%= 'expanded' if @enhanced_query[category.to_sym].present? %>"
<button class="filter-label <%= 'expanded' if @enhanced_query[category.to_sym].present? || first == true %>"
onClick="toggleFilter(this)"><%= nice_labels[category] || category %></button>
<div class="filter-options">
<ul class="category-terms list-unbulleted">
Expand Down
29 changes: 16 additions & 13 deletions app/views/search/results.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,22 @@
<%= render(partial: 'shared/error', collection: @errors) %>

<div class="<%= 'layout-1q3q' if @filters.present? %> layout-band top-space">
<% if @filters.present? %>
<aside class="col1q filter-container">
<div id="filters">
<h2 class="hd-3">Filter your results</h2>
<h3 class="hd-4"><em><%= results_summary(@pagination[:hits]) %></em></h3>
<% @filters&.each_with_index do |(category, values), index| %>
<% if index == 0 %>
<%= render(partial: 'search/filter', locals: {category: category, values: values, first: true}) %>
<% else %>
<%= render(partial: 'search/filter', locals: {category: category, values: values, first: false }) %>
<% end %>
<% end %>
</div>
</aside>
<% end %>

<div class="col3q wrap-results">
<% if @results.present? %>
<ul id="results" class="list-unbulleted">
Expand All @@ -61,19 +77,6 @@
<% end %>
</div>

<% if @filters.present? %>
<aside class="col1q filter-container">
<div id="filters">
<h2 class="hd-3">Filter your results</h2>
<h3 class="hd-4"><em><%= @pagination[:hits] %> items</em></h3>
<% @filters&.each do |category, values| %>
<%= render(partial: 'search/filter', locals: {category: category, values: values}) %>
<% end %>
</div>
</aside>
<% end %>
</div>

<% if @results.present? %>
<div id="pagination">
<%= render partial: "pagination" %>
Expand Down
20 changes: 20 additions & 0 deletions test/helpers/results_helper_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
require 'test_helper'

class ResultsHelperTest < ActionView::TestCase
include ResultsHelper

test 'if number of hits is equal to 10,000, results summary returns "10,000+"' do
hits = 10000
assert_equal '10,000+ items', results_summary(hits)
end

test 'if number of hits is above 10,000, results summary returns "10,000+"' do
hits = 10500
assert_equal '10,000+ items', results_summary(hits)
end

test 'if number of hits is below 10,000, results summary returns actual number of results' do
hits = 9000
assert_equal '9,000 items', results_summary(hits)
end
end

0 comments on commit 00608d6

Please sign in to comment.