Skip to content

Commit e69f1fd

Browse files
Define new helper for tab links, with aria
** Why are these changes being introduced: We want to add an `aria-current="page"` attribute to the currently active tab link on a results page, so that assistive technologies can parse which is the active tab. This requires a conditional link_to call, as well as an update to the javascript which handles tab changes. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-180 ** How does this address that need: Because we can't (apparently) write a conditional inside an argument to a link_to call, we have to maintain the conditional check elsewhere - so to prevent the source_tabs partial from becoming a mess of multiple conditionas, we define a new search helper function, link_to_tab. This takes a string as an argument, and is capable of outputting both a link to the active tab, as well as to an inactive tab. We also update loading_spinner.js to be able to maintain the state of the aria-current attribute. ** Document any side effects to this change: The tests for this helper function _should_ be robust enough, but I worry that they're a bit too clever. I didn't want to just write a string comparison assertion, because that feels very fragile given how much this partial has been changing lately. The assertions as written should focus on the values being present or not, but still be robust to other changes. Also note that, for now, the source_tabs partial is still a bit bespoke in that there are explicitly two calls to the various tabs. I'd originally intended to define a list in the controller, but managing the list of tabs in this way feels awkward and like it doesn't gain anything since the tabs are entwined with so much of the rest of the application. Abstracting the tab order and contents to a list doesn't save us from other complexity.
1 parent 3f1acf8 commit e69f1fd

File tree

4 files changed

+41
-11
lines changed

4 files changed

+41
-11
lines changed

app/helpers/search_helper.rb

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,19 @@ def link_to_result(result)
2222
end
2323
end
2424

25+
def link_to_tab(target)
26+
if @active_tab == target.downcase
27+
link_to target, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: target.downcase)),
28+
aria: { current: "page" },
29+
class: "active tab-link",
30+
data: { turbo_frame: "search-results", turbo_action: "advance" }
31+
else
32+
link_to target, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: target.downcase)),
33+
class: "tab-link",
34+
data: { turbo_frame: "search-results", turbo_action: "advance" }
35+
end
36+
end
37+
2538
def view_online(result)
2639
return unless result[:source_link].present?
2740

app/javascript/loading_spinner.js

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,15 +23,17 @@ document.addEventListener('turbo:frame-render', function(event) {
2323
// console.log(`Updated tab input value to: ${queryParam}`);
2424
}
2525

26-
// update active tab styling
27-
// remove active class from all tabs
26+
// update tab links to reflect new state. This is a two-step process:
27+
// 1. Reset all tabs to base condition
2828
document.querySelectorAll('.tab-link').forEach((tab) => {
2929
tab.classList.remove('active');
30+
tab.removeAttribute('aria-current');
3031
});
31-
// add active class to current tab
32+
// 2. Add "active" class and aria-current attribute to the newly-active tab link
3233
const currentTabLink = document.querySelector(`.tab-link[href*="tab=${queryParam}"]`);
3334
if (currentTabLink) {
3435
currentTabLink.classList.add('active');
36+
currentTabLink.setAttribute('aria-current', 'page');
3537
}
3638

3739
// Clear the pending action
Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,8 @@
11
<!-- Tab Navigation -->
22
<nav id="tabs" class="tab-navigation" aria-label="Result type navigation">
33
<ul>
4-
<li>
5-
<%= link_to "Primo", results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: 'primo')),
6-
class: "tab-link #{'active' if @active_tab == 'primo'}",
7-
data: { turbo_frame: "search-results", turbo_action: "advance" } %></li>
8-
<li>
9-
<%= link_to "TIMDEX", results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: 'timdex')),
10-
class: "tab-link #{'active' if @active_tab == 'timdex'}",
11-
data: { turbo_frame: "search-results", turbo_action: "advance" } %></li>
4+
<li><%= link_to_tab("Primo") %></li>
5+
<li><%= link_to_tab("TIMDEX") %></li>
126
</ul>
137
</nav>
148

test/helpers/search_helper_test.rb

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,27 @@ class SearchHelperTest < ActionView::TestCase
199199
assert_equal 'Sample Document Title', link_to_result(result)
200200
end
201201

202+
test 'link_to_tab builds a link without an aria attribute when that tab is not active' do
203+
@active_tab = 'bar'
204+
actual = link_to_tab('Foo')
205+
206+
assert_select Nokogiri::HTML::Document.parse( actual ), 'a' do |link|
207+
assert_select '[class*=?]', 'active', count: 0
208+
assert_select '[class*=?]', 'tab-link'
209+
assert_select '[aria-current=?]', 'page', count: 0
210+
end
211+
end
212+
213+
test 'link_to_tab builds a link that includes an aria attribute when that tab is active' do
214+
@active_tab = 'foo'
215+
actual = link_to_tab('Foo')
216+
217+
assert_select Nokogiri::HTML::Document.parse( actual ), 'a' do |link|
218+
assert_select link, '[class*=?]', 'active'
219+
assert_select link, '[aria-current=?]', 'page', count: 1
220+
end
221+
end
222+
202223
test 'primo_search_url generates correct Primo URL' do
203224
query = 'machine learning'
204225
expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/search?query=any%2Ccontains%2Cmachine+learning&vid=01MIT_INST%3AMIT'

0 commit comments

Comments
 (0)