-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
Fix duplicate search filters #1180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ty for catching this! Looks good and the a11y changes are all intact. Just got one change in there
<a href="#" class="filter-value" data-param="sponsorships_exact:{{name}}" title="{{ name|title }}"> | ||
{% if name in selected_facets.sponsorships %} | ||
<div class="skinny-list"> | ||
<a href="#" class="filter-value" data-param="sponsorships_exact:{{name}}" title="{{ name|title }}"> | ||
<strong>{{ name | title }}</strong> | ||
</a> | ||
<a href="#" class="remove-filter-value btn btn-primary btn-sm" data-param="{{facet_name}}_exact:{{name}}" aria-label="Remove {{name}} filter"> | ||
<i class="fa fa-times" aria-hidden="true"></i> | ||
</a> | ||
</div> | ||
<strong>{{ name | title }}</strong> | ||
<a href="#" class="remove-filter-value btn btn-primary btn-sm" data-param="{{facet_name}}_exact:{{name}}" aria-label="Remove {{name}} filter"> | ||
<i class="fa fa-times" aria-hidden="true"></i> | ||
</a> | ||
{% else %} | ||
<a href="#" class="filter-value" data-param="sponsorships_exact:{{name}}" title="{{ name|title }}"> | ||
{{ name | title }} | ||
</a> | ||
{% endif %} | ||
|
||
<span class="badge badge-facet ms-auto">{{ count }}</span> | ||
|
||
</li> | ||
{% endif%} | ||
|
||
{% endfor %} | ||
{% else %} | ||
|
||
{% for name, count in item_list %} | ||
{% if count %} | ||
{% include 'search/_ordered_search_filter_items.html' %} | ||
</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Voiceover is reading each facet link in the Meetings filter twice, and it looks like this produces nested links (which then resolves to just two links next to each other).
Removing the outermost anchor tag might be what we're looking for here. Maybe something like:
<div class="skinny-list">
{% if name in selected_facets.sponsorships %}
<strong>{{ name | title }}</strong>
<a href="#" class="remove-filter-value btn btn-primary btn-sm" data-param="{{facet_name}}_exact:{{name}}" aria-label="Remove {{name}} filter">
<i class="fa fa-times" aria-hidden="true"></i>
</a>
{% else %}
<a href="#" class="filter-value" data-param="sponsorships_exact:{{name}}" title="{{ name|title }}">
{{ name | title }}
</a>
{% endif %}
</div>
Great catch, thank you @xmedr! Patch in and ready for your eyeballs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is perfect!
Overview
The accessibility changes did not address an issue where search filters were duplicated. This PR applies a patch with the updated markup.
Connects #1009, #979
Demo
Before (Staging)
After
Testing Instructions