Fix form required attributes redundancy and keyword input description #195
Conversation
Pull Request Test Coverage Report for Build 9320855317Details
💛 - Coveralls |
|
CodeClimate issues are valid and noted, but not new. I think they showed up again because I touched the offending JS, so it analyzed it again instead of ignoring it. |
|
Rich has confirmed that this review app fixes the last of the a11y issues. Opening for review. |
| @@ -30,7 +30,8 @@ end | |||
| <input id="basic-search-main" type="search" | |||
| class="field field-text basic-search-input <%= "required" if search_required %>" name="q" | |||
| title="Keyword anywhere" placeholder="Enter your search" | |||
| value="<%= params[:q] %>" <%= 'required="required" aria-required="true"' if search_required %>> | |||
| value="<%= params[:q] %>" <%= 'required' if search_required %> | |||
| <%= 'aria-describedby=site-desc' if Flipflop.enabled?(:gdt) %>> | |||
There was a problem hiding this comment.
Can you help me understand why this is only being used for GDT enabled sites? I noted you called this out as unnecessary for non-GDT in the commit message but I don't understand why. I think I see the differences, but it's unclear why it was only an issue in GDT.
There was a problem hiding this comment.
The relative clarity of the non-GDT form is the tiebreaker for me. It's just a keyword input with some advanced fields, so I suspect it will be clearer if a screen reader user goes from anh2 with the text of 'Search the MIT Libraries' to a 'Keyword anywhere' input.
To be transparent, though, this is an assumption I've made. I did not ask Rich to test search.libraries.mit.edu, and it's possible he would have the same feedback on that application. If that's a concern, I'd be happy add some copy (with Darcy's input) to describe the keyword input for non-GDT apps.
There was a problem hiding this comment.
I'm fine with this solution for now as GDT is the only prod app. I'm waffly on whether we should pre-address this for non-GDT apps or remember/expect to come back to it when we prep non-GDT apps for prod. I'll go ahead and approve this.
There was a problem hiding this comment.
Sounds good. It certainly calls to mind your idea of a post-project analysis of what are actually GDT features vs. non-GDT features.
Why these changes are being introduced: DAS has informed us that it is bad practice to use both `required` and `aria-required`. They also suggested that we add an `aria-describedby` to the keyword input that links to the `p` tag below the site title. Relevant ticket(s): * [GDT-327](https://mitlibraries.atlassian.net/browse/GDT-327) How this addresses that need: This removes the redundant `aria-required` attributes and makes the suggested labeling change for the keyword input. Side effects of this change: The labeling piece only affects GDT, as non-GDT apps don't currently use the `site_title` partial. This feels acceptable, because the keyword input is more clearly labeled in other TIMDEX UI applications.
Why these changes are being introduced:
DAS has informed us that it is bad practice to use both
requiredandaria-required. They also suggested that we add anaria-describedbyto the keyword input that links to theptag below the site title.Relevant ticket(s):
How this addresses that need:
This removes the redundant
aria-requiredattributes and makes the suggested labeling change for the keyword input.Side effects of this change:
The labeling piece only affects GDT, as non-GDT apps don't currently use the
site_titlepartial. This feels acceptable, because the keyword input is more clearly labeled in other TIMDEX UI applications.Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
Confirm that required fields are still required (i.e., built-in form validations fire when they are empty).
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing