Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve accessibility of search form #188

Merged
merged 1 commit into from
May 10, 2024
Merged

Improve accessibility of search form #188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions app/assets/stylesheets/partials/_filters.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
#filters {
@media (max-width: $bp-screen-md) {
margin-bottom: 1.2rem;
}
details.filter-category {
position: relative;
margin-bottom: 0.5em;
Expand Down
2 changes: 1 addition & 1 deletion app/assets/stylesheets/partials/_panels.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
}
}
.list-filter-summary {
padding-top: 1rem;
padding-top: 0.8rem;
border-top: 1px solid $black;
}
.hd-search-summary {
Expand Down
5 changes: 4 additions & 1 deletion app/assets/stylesheets/partials/_results.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
.top-space {
margin-top: 2.4rem;
margin-top: 1.2rem;
@media (min-width: $bp-screen-md) {
margin-top: 2.4rem;
}
}

.wrap-results {
Expand Down
16 changes: 4 additions & 12 deletions app/assets/stylesheets/partials/_search.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
.basic-search {
background-color: #989898;
margin-bottom: 0rem;
padding: 2.4rem 2rem 1rem 2.4rem;
padding: 2.4rem 2rem 1.6rem 2rem;

details {
&:first-of-type {
Expand Down Expand Up @@ -35,12 +35,7 @@
width: 100%;
margin-bottom: .8rem;
padding: 6px 12px;

@media (min-width: $bp-screen-sm) {
display: inline-block;
width: 80%;
margin-bottom: 0;
}
margin-bottom: 0;
}

summary {
Expand Down Expand Up @@ -112,12 +107,9 @@
}

.basic-search-submit {
@media (min-width: $bp-screen-sm) {
display: inline-block;
width: 18%;
}
width: 150px;
margin-top: 0.8rem;
.btn {
border-radius: 0;
width: 100%;
}
}
Expand Down
17 changes: 6 additions & 11 deletions app/views/search/_form.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,4 @@
<%
# Conditional input label depends on whether a search has already happened.
label = "Search the MIT Libraries"
if params[:q]
label = "You searched for"
end

# Initial form setup is determined by the advanced parameter. Thereafter it is altered by javascript.
advanced_label = "Search by title, author, etc."
advanced_label_class = "closed"
Expand Down Expand Up @@ -34,12 +28,9 @@ end
<form id="basic-search" class="form-horizontal basic-search" action="<%= results_path %>" method="get" role="search">
<div class="form-group">
<input id="basic-search-main" type="search"
class="field field-text basic-search-input <%= "required" if search_required %>" name="q" title="<%= label %>"
placeholder="Enter your search"
class="field field-text basic-search-input <%= "required" if search_required %>" name="q"
title="Keyword anywhere" placeholder="Enter your search"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't noticed previously that when we expand to show additional form fields we change the placeholder text for this field. I understand why we do that, but it feels a bit odd and we may want to revisit (later) how to handle this (including possibly just throwing "advanced search" into it's own view which I don't love but does make things simpler for a few edge cases).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We used to do it for all of the dropdowns as well. I think it's bad practice (usability testing confirmed that suspicion), and I'm taking this opportunity to remove the last instance of it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify what you commented here? I still see this behavior on this PR for all of the various expandable form components. Are you referring to something other than the placeholder for keyword searching changing when expanding/collapsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was actually talking about labels, not placeholder text. We used to change the text of the dropdown labels when they were open (e.g., "Geospatial bounding box search" became "Close geospatial bounding box search"). The keyword input label was the last instance of that, changed here.

I don't care as much about placeholder text, but I'd be perfectly happy to leave it as 'Keyword anywhere' in all form states. I'm not sure 'Enter your search' is adding any value here.

Sorry for the confusion; I only realized after your question how confusing my comment was. 🙂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, I see what you were referring to and yes that has been a good change.

I don't feel strongly enough about the placeholder text to override any previous decisions on it. I find it unexpected, but it isn't inherently wrong.

Copy link
Contributor Author

@jazairi jazairi May 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not inherently wrong, no, but I get the sense that 'Keyword anywhere' is more useful regardless of form state. (But, like you, probably not enough to make a change here.)

value="<%= params[:q] %>" <%= 'required="required" aria-required="true"' if search_required %>>
<div class="basic-search-submit">
<button type="submit" class="btn button-primary">Search</button>
</div>

<% if Flipflop.enabled?(:gdt) %>
<details id="geobox-search-panel" <%= "open" if params[:geobox] == "true" %>>
Expand Down Expand Up @@ -220,6 +211,10 @@ end
</div>
</details>
</div>

<div class="basic-search-submit">
<button type="submit" class="btn button-primary">Search</button>
jazairi marked this conversation as resolved.
Show resolved Hide resolved
</div>
</form>

<% if Flipflop.enabled?(:boolean_picker) %>
Expand Down
Loading