Skip to content

Commit ee75285

Browse files
jazairijazairi
committed
Refactor search form JS
Why these changes are being introduced: During usability testing, stakeholders noted that the form can enter a state in which they keyword field is incorrectly required. (E.g., the geodistance panel is open, but the keyword input still has the `required` attribute.) In our attempts to replicate this bug, we have intermittently gotten the form into a similar out-of-sync state. However, we have not been able to replicate it consistently. Part of the problem is the poor structure of the search form JS, which makes the script difficult to debug. Relevant ticket(s): * [GDT-302](https://mitlibraries.atlassian.net/browse/GDT-302) * [TIMX-281](https://mitlibraries.atlassian.net/browse/TIMX-281) How this addresses that need: This refactors the search form JS to be more well structured, concise, and DRY, with the intention of resolving two tickets: * GDT-302, the bug mentioned above. To resolve this, we delay the execution of the functions that trigger the keyword input state change, so as to avoid the syncing issues we found in testing. * TIMX-281, a bug in non-GDT form state that prevents the visual changes to icon and placeholder when the advanced search panel is open. Because we can't consistently replicate the bug that GDT-302 seeks to address, I can't say with certainty that this fixes the problem. I've tried click-testing it, and certain behavior that had previously gotten it out of sync no longer does. At minimum, the refactor should improve the maintainability of our codebase and make it easier to debug any future issues that may emerge. Side effects of this change: `var` is intentionally used instead of `const` or `let`. When form submission fails with a redirect, Rails seems not to trigger a full page reload. This causes the JS interpreter to throw an error for modifying variables that have already been declare, unless we declare them with `var`. Since `var` is still valid JS, albeit not ideal, it's preferable at this point to overriding Rails (likely Turbo) behavior on form submit.
1 parent 03f29ec commit ee75285

File tree

3 files changed

+92
-127
lines changed

3 files changed

+92
-127
lines changed

‎app/assets/stylesheets/partials/_search.scss‎

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,14 +48,6 @@
4848
font-family: FontAwesome;
4949
margin-right: 1rem;
5050
}
51-
52-
&.closed:before {
53-
content: '\f054';
54-
}
55-
56-
&.open:before {
57-
content: '\f078';
58-
}
5951
}
6052

6153
&::-webkit-details-marker {
@@ -71,6 +63,22 @@
7163
color: $black;
7264
}
7365
}
66+
#advanced-search-label,
67+
#geobox-search-label,
68+
#geodistance-search-label {
69+
&::before {
70+
content: '\f054';
71+
}
72+
}
73+
&[open] {
74+
#advanced-search-label,
75+
#geobox-search-label,
76+
#geodistance-search-label {
77+
&::before {
78+
content: '\f078';
79+
}
80+
}
81+
}
7482
}
7583

7684
#geobox-search-panel,

‎app/javascript/search_form.js‎

Lines changed: 60 additions & 103 deletions
Original file line numberDiff line numberDiff line change
@@ -1,123 +1,80 @@
1-
function disableAdvanced() {
2-
advanced_field.setAttribute('value', '');
3-
if (geobox_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
4-
keyword_field.toggleAttribute('required');
5-
keyword_field.classList.toggle('required');
6-
keyword_field.setAttribute('placeholder', 'Enter your search');
7-
}
8-
[...details_panel.getElementsByClassName('field')].forEach(
9-
field => field.value = ''
10-
);
11-
advanced_label.classList = 'closed';
12-
};
1+
var keywordField = document.getElementById('basic-search-main');
2+
var advancedPanel = document.getElementById('advanced-search-panel');
3+
var geoboxPanel = document.getElementById('geobox-search-panel');
4+
var geodistancePanel = document.getElementById('geodistance-search-panel');
5+
var allPanels = document.getElementsByClassName('form-panel');
136

14-
function enableAdvanced() {
15-
advanced_field.setAttribute('value', 'true');
16-
if (geobox_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
17-
keyword_field.toggleAttribute('required');
18-
keyword_field.classList.toggle('required');
19-
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
7+
function togglePanelState(currentPanel) {
8+
// Only the geoboxPanel and geodistancePanel inputs need to be required. Advanced search inputs do not.
9+
if (currentPanel === geoboxPanel || currentPanel === geodistancePanel) {
10+
toggleRequiredFieldset(currentPanel);
2011
}
21-
advanced_label.classList = 'open';
22-
};
2312

24-
function disableGeobox() {
25-
if (advanced_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
26-
keyword_field.toggleAttribute('required');
27-
keyword_field.classList.toggle('required');
28-
keyword_field.setAttribute('placeholder', 'Enter your search');
29-
}
30-
geobox_field.setAttribute('value', '');
31-
[...geobox_details_panel.getElementsByClassName('field')].forEach(function(field) {
32-
field.value = '';
33-
field.classList.toggle('required');
34-
field.toggleAttribute('required');
35-
});
36-
geobox_label.classList = 'closed';
37-
};
13+
// These two functions are delayed to ensure that events have propagated first. Otherwise, they will fire before
14+
// the `open` attribute has toggled on the currentPanel, resulting in unexpected behavior.
15+
setTimeout(toggleKeywordRequired, 0);
16+
setTimeout(updateKeywordPlaceholder, 0);
3817

39-
function enableGeobox() {
40-
if (advanced_label.classList.contains('closed') && geodistance_label.classList.contains('closed')) {
41-
keyword_field.toggleAttribute('required');
42-
keyword_field.classList.toggle('required');
43-
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
44-
}
45-
geobox_field.setAttribute('value', 'true');
46-
[...geobox_details_panel.getElementsByClassName('field')].forEach(function(field) {
47-
field.value = '';
48-
field.classList.toggle('required');
49-
field.toggleAttribute('required');
50-
});
51-
geobox_label.classList = 'open';
52-
};
18+
// Finally, enable or disable the search type of the current panel, based on whether it is open or not.
19+
toggleSearch(currentPanel);
20+
}
5321

54-
function disableGeodistance() {
55-
if (advanced_label.classList.contains('closed') && geobox_label.classList.contains('closed')) {
56-
keyword_field.toggleAttribute('required');
57-
keyword_field.classList.toggle('required');
58-
keyword_field.setAttribute('placeholder', 'Enter your search');
59-
}
60-
geodistance_field.setAttribute('value', '');
61-
[...geodistance_details_panel.getElementsByClassName('field')].forEach(function(field) {
22+
function toggleRequiredFieldset(panel) {
23+
[...panel.getElementsByClassName('field')].forEach((field) => {
6224
field.value = '';
6325
field.classList.toggle('required');
6426
field.toggleAttribute('required');
6527
});
66-
geodistance_label.classList = 'closed';
67-
};
28+
}
6829

69-
function enableGeodistance() {
70-
if (advanced_label.classList.contains('closed') && geobox_label.classList.contains('closed')) {
71-
keyword_field.toggleAttribute('required');
72-
keyword_field.classList.toggle('required');
73-
keyword_field.setAttribute('placeholder', 'Keyword anywhere');
74-
}
75-
geodistance_field.setAttribute('value', 'true');
76-
[...geodistance_details_panel.getElementsByClassName('field')].forEach(function(field) {
77-
field.value = '';
78-
field.classList.toggle('required');
79-
field.toggleAttribute('required');
80-
});
81-
geodistance_label.classList = 'open';
82-
};
83-
84-
85-
var advanced_field = document.getElementById('advanced-search-field');
86-
var advanced_label = document.getElementById('advanced-search-label');
87-
var advanced_toggle = document.getElementById('advanced-summary');
88-
var details_panel = document.getElementById('advanced-search-panel');
89-
var keyword_field = document.getElementById('basic-search-main');
90-
var geobox_field = document.getElementById('geobox-search-field');
91-
var geobox_label = document.getElementById('geobox-search-label');
92-
var geobox_toggle = document.getElementById('geobox-summary');
93-
var geobox_details_panel = document.getElementById('geobox-search-panel');
94-
var geodistance_field = document.getElementById('geodistance-search-field');
95-
var geodistance_label = document.getElementById('geodistance-search-label');
96-
var geodistance_toggle = document.getElementById('geodistance-summary');
97-
var geodistance_details_panel = document.getElementById('geodistance-search-panel');
98-
99-
geobox_toggle.addEventListener('click', event => {
100-
if (geobox_details_panel.attributes.hasOwnProperty('open')) {
101-
disableGeobox();
30+
// Each panel has a hidden input that, when true, enables that type of search.
31+
function toggleSearch(panel) {
32+
let input = panel.querySelector('.fieldset-toggle');
33+
if (panel.open) {
34+
input.setAttribute('value', '');
10235
} else {
103-
enableGeobox();
36+
input.setAttribute('value', 'true');
10437
}
105-
});
38+
}
10639

107-
geodistance_toggle.addEventListener('click', event => {
108-
if (geodistance_details_panel.attributes.hasOwnProperty('open')) {
109-
disableGeodistance();
40+
// The keyword field is required only if all panels are closed.
41+
function toggleKeywordRequired() {
42+
if (Array.from(allPanels).every((panel) => !panel.open)) {
43+
keywordField.setAttribute('required', '');
44+
keywordField.classList.add('required');
11045
} else {
111-
enableGeodistance();
46+
keywordField.removeAttribute('required');
47+
keywordField.classList.remove('required');
11248
}
113-
});
49+
}
11450

115-
advanced_toggle.addEventListener('click', event => {
116-
if (details_panel.attributes.hasOwnProperty('open')) {
117-
disableAdvanced();
51+
// Placeholder text should be 'Keyword anywhere' if any panels are open, and 'Enter your search' otherwise.
52+
function updateKeywordPlaceholder() {
53+
if (Array.from(allPanels).some((panel) => panel.open)) {
54+
keywordField.setAttribute('placeholder', 'Keyword anywhere');
11855
} else {
119-
enableAdvanced();
56+
keywordField.setAttribute('placeholder', 'Enter your search');
12057
}
121-
});
58+
}
59+
60+
// Add event listeners for all panels in the DOM. For GDT, this is currently both geospatial panels and the advanced
61+
// panel. In all other TIMDEX UI apps, it's just the advanced panel.
62+
if (Array.from(allPanels).includes(geoboxPanel && geodistancePanel)) {
63+
document.getElementById('geobox-summary').addEventListener('click', () => {
64+
togglePanelState(geoboxPanel);
65+
});
66+
67+
document.getElementById('geodistance-summary').addEventListener('click', () => {
68+
togglePanelState(geodistancePanel);
69+
});
70+
71+
document.getElementById('advanced-summary').addEventListener('click', () => {
72+
togglePanelState(advancedPanel);
73+
});
74+
} else {
75+
document.getElementById('advanced-summary').addEventListener('click', () => {
76+
togglePanelState(advancedPanel);
77+
});
78+
}
12279

12380
console.log('search_form.js loaded');

‎app/views/search/_form.html.erb‎

Lines changed: 16 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,44 +1,42 @@
11
<%
22
# Initial form setup is determined by the advanced parameter. Thereafter it is altered by javascript.
33
advanced_label = "Search by title, author, etc."
4-
advanced_label_class = "closed"
54
search_required = true
65
if params[:advanced] == "true"
7-
advanced_label_class = "open"
86
search_required = false
97
end
108

119
geobox_label = "Geospatial bounding box search"
12-
geobox_label_class = "closed"
1310
if params[:geobox] == "true"
14-
geobox_label_class = "open"
1511
geobox_required = true
1612
search_required = false
1713
end
1814

1915
geodistance_label = "Geospatial distance search"
20-
geodistance_label_class = "closed"
2116
if params[:geodistance] == "true"
22-
geodistance_label_class = "open"
2317
geodistance_required = true
2418
search_required = false
2519
end
20+
21+
# Placeholder text for the keyword input changes if any of the search panels are open.
22+
keyword_placeholder = search_required ? "Enter your search" : "Keyword anywhere"
2623
%>
2724

2825
<form id="basic-search" class="form-horizontal basic-search" action="<%= results_path %>" method="get" role="search">
2926
<div class="form-group">
3027
<input id="basic-search-main" type="search"
3128
class="field field-text basic-search-input <%= "required" if search_required %>" name="q"
32-
title="Keyword anywhere" placeholder="Enter your search"
29+
title="Keyword anywhere" placeholder="<%= keyword_placeholder %>"
3330
value="<%= params[:q] %>" <%= 'required' if search_required %>
3431
<%= 'aria-describedby=site-desc' if Flipflop.enabled?(:gdt) %>>
3532

3633
<% if Flipflop.enabled?(:gdt) %>
37-
<details id="geobox-search-panel" <%= "open" if params[:geobox] == "true" %>>
34+
<details id="geobox-search-panel" class="form-panel" <%= "open" if params[:geobox] == "true" %>>
3835
<summary class="btn button-secondary" id="geobox-summary">
39-
<span id="geobox-search-label" class="<%= geobox_label_class %>"><%= geobox_label %></span>
36+
<span id="geobox-search-label"><%= geobox_label %></span>
4037
</summary>
41-
<input id="geobox-search-field" type="hidden" name="geobox" value="<%= params[:geobox] %>">
38+
<input id="geobox-search-field" class="fieldset-toggle" type="hidden" name="geobox"
39+
value="<%= params[:geobox] %>">
4240
<fieldset>
4341
<legend>Search within a geospatial bounding box</legend>
4442
<p>* All fields in this section are required</p>
@@ -94,11 +92,12 @@ end
9492
</div>
9593
</fieldset>
9694
</details>
97-
<details id="geodistance-search-panel" <%= "open" if params[:geodistance] == "true" %>>
95+
<details id="geodistance-search-panel" class="form-panel" <%= "open" if params[:geodistance] == "true" %>>
9896
<summary class="btn button-secondary" id="geodistance-summary">
99-
<span id="geodistance-search-label" class="<%= geodistance_label_class %>"><%= geodistance_label %></span>
97+
<span id="geodistance-search-label"><%= geodistance_label %></span>
10098
</summary>
101-
<input id="geodistance-search-field" type="hidden" name="geodistance" value="<%= params[:geodistance] %>">
99+
<input id="geodistance-search-field" class="fieldset-toggle" type="hidden" name="geodistance"
100+
value="<%= params[:geodistance] %>">
102101
<fieldset>
103102
<legend>Search within a distance of a geographic point</legend>
104103
<p>* All fields in this section are required</p>
@@ -145,11 +144,12 @@ end
145144
</details>
146145
<% end %>
147146

148-
<details id="advanced-search-panel" <%= "open" if params[:advanced] == "true" %>>
147+
<details id="advanced-search-panel" class="form-panel" <%= "open" if params[:advanced] == "true" %>>
149148
<summary class="btn button-secondary" id="advanced-summary">
150-
<span id="advanced-search-label"class="<%= advanced_label_class %>"><%= advanced_label %></span>
149+
<span id="advanced-search-label"><%= advanced_label %></span>
151150
</summary>
152-
<input id="advanced-search-field" type="hidden" name="advanced" value="<%= params[:advanced] %>">
151+
<input id="advanced-search-field" class="fieldset-toggle" type="hidden" name="advanced"
152+
value="<%= params[:advanced] %>">
153153
<div class="field-container">
154154
<div class="field-wrap">
155155
<label for="advanced-title" class="field-label">Title</label>

0 commit comments

Comments
 (0)