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.

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

Merged
merged 2 commits into from
Dec 4, 2024
Merged

Fix duplicate search filters #1180

merged 2 commits into from
Dec 4, 2024

Conversation

hancush
Copy link
Collaborator

@hancush hancush commented Dec 3, 2024

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)

Screenshot 2024-12-03 at 11 27 18 AM

After

Screenshot 2024-12-03 at 11 26 40 AM

Testing Instructions

  • On the review app, view the search page and confirm all of the facets display correctly.
  • Double check that the markup retains the accessibility changes (namely, data attributes and the aria-* attributes).

@hancush hancush requested a review from xmedr December 3, 2024 17:29
@fgregg fgregg temporarily deployed to la-metro-cou-hcg-search-3n8xls December 3, 2024 17:32 Inactive
Copy link
Collaborator

@xmedr xmedr left a 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

Comment on lines 34 to 45
<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>
Copy link
Collaborator

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>

@hancush hancush requested a review from xmedr December 3, 2024 20:20
@hancush hancush temporarily deployed to la-metro-cou-hcg-search-3n8xls December 3, 2024 20:20 Inactive
@hancush
Copy link
Collaborator Author

hancush commented Dec 3, 2024

Great catch, thank you @xmedr! Patch in and ready for your eyeballs.

Copy link
Collaborator

@xmedr xmedr left a comment

Choose a reason for hiding this comment

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

This is perfect!

@hancush hancush merged commit 32ea9dc into main Dec 4, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants