Contrib.social

Refactor the Search component to treat filter changes generically

Open on Drupal.org →
Created on 28 January 2025, 3 months ago

Problem/Motivation

The Search component is a mess, largely because it does not treat the filters generically.

It considers the text search different from the category checkboxes, and those different from the maintenance/development/security status (which it collectively considers as a single group). This architecture is kind of nonsensical and makes it nearly impossible for sources to define their own set of filters.

Proposed resolution

For now, let's ignore the text search -- it'll need more work, later on. But the other filters need to be treated generically, regardless of whether it is a multiple choice filter (categories), or a boolean filter (such as development status).

When any of these is changed, only a single event should be dispatched -- onFilterChange. Its event details should include the complete current set of filter values. Example pseudocode:

function onFilterChange(event) {
  // event.detail should look like this:
  assert(event.detail).looksLike({
    'development_status': false, // current filter value
    'some_other_filter': true, // current filter value
    'categories': ['foo', 'bar', ...], // current filter value
  })
}

This should result in onAdvancedSearch() and onCategorySelect being completely removed.

📌 Task
Status

Active

Version

2.0

Component

Code

Created by

🇺🇸United States phenaproxima Massachusetts

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @phenaproxima
  • Comment 3 months ago →
    🇺🇸United States phenaproxima Massachusetts
  • Comment 3 months ago →
    🇺🇸United States phenaproxima Massachusetts
  • First commit to issue fork.
  • Merge request !699#3502935: Refactor the Search component to treat filter changes generically → (Merged) created by utkarsh_33
  • Pipeline finished with Canceled
    3 months ago
    Total: 93s
    #409975
  • Pipeline finished with Failed
    3 months ago
    Total: 514s
    #409976
  • Pipeline finished with Failed
    3 months ago
    Total: 615s
    #410021
  • Pipeline finished with Failed
    3 months ago
    Total: 406s
    #410030
  • Pipeline finished with Failed
    3 months ago
    Total: 389s
    #410147
  • Pipeline finished with Failed
    3 months ago
    Total: 412s
    #410172
  • Pipeline finished with Success
    3 months ago
    Total: 385s
    #410195
  • Comment 3 months ago →
    🇮🇳India utkarsh_33

    The tests are all green which means functionally everything is working as expected.I have a done some basic restructuring for now (which might be silly) but i think it's a good start for now.I'll ask @phenaproxima to guide me more on this with what i need to follow in order to achieve the goal.

  • Comment 3 months ago →
    🇮🇳India utkarsh_33

    I will discuss this with @phenaproxima and update the status of the issue soon.

  • Pipeline finished with Success
    3 months ago
    Total: 482s
    #413189
  • Pipeline finished with Failed
    3 months ago
    Total: 703s
    #414206
  • Pipeline finished with Success
    3 months ago
    Total: 476s
    #414210
  • Comment 3 months ago →
    🇮🇳India utkarsh_33

    I think this is now ready and changes are inline with the discussion that i had with @phenaproxima.Marking it NR .

  • Comment 3 months ago →
    🇮🇳India narendraR Jaipur, India

    Tested manually and it seems that On selecting multiple categories, it does not announce the Count of selected categories when voiceover is on.

  • Comment 3 months ago →
    🇺🇸United States phenaproxima Massachusetts

    This generally looks good. I think we can remove filtersWrapper, it seems like a pointless extra layer around what $filters is already doing.

    We do need to fix the announce problem that @narendrar found. I would suggest that we change the backend MultipleChoiceFilter class so that it accepts a "template string" that can be used for announcements, and then have the announce function in Svelte use that. Make it part of the filter's definition -- how does it want to announce a state change to the user? All the filters should be taking this sort of accessibility into account, but let's fix it for the MultipleChoiceFilter type first, and the other filter types later.

  • Pipeline finished with Success
    3 months ago
    Total: 475s
    #415305
  • Comment 3 months ago →
    🇮🇳India utkarsh_33
  • Comment 3 months ago →
    🇮🇳India narendraR Jaipur, India

    Re #9, This intermittent announce issue seems to exist on 2.0.x also and can be skipped for now.

  • Comment 3 months ago →
    🇺🇸United States phenaproxima Massachusetts

    This is making a ton of sense. There are a few more steps to go, but this is really solid so far!

  • Pipeline finished with Canceled
    3 months ago
    Total: 90s
    #416539
  • Pipeline finished with Failed
    3 months ago
    Total: 719s
    #416543
  • Pipeline finished with Failed
    3 months ago
    Total: 635s
    #416557
  • Pipeline finished with Canceled
    3 months ago
    Total: 136s
    #416566
  • Pipeline finished with Failed
    3 months ago
    Total: 612s
    #416567
  • Pipeline finished with Failed
    3 months ago
    Total: 594s
    #416582
  • Pipeline finished with Failed
    3 months ago
    Total: 721s
    #416588
  • Pipeline finished with Failed
    3 months ago
    Total: 693s
    #416605
  • Comment 3 months ago →
    🇮🇳India utkarsh_33

    I am facing issues in solving the test fails.I checked the URLs formed in both 2.0.x and this branch are same.

  • Pipeline finished with Canceled
    3 months ago
    Total: 425s
    #416919
  • Pipeline finished with Failed
    3 months ago
    Total: 596s
    #416925
  • Pipeline finished with Failed
    3 months ago
    Total: 696s
    #416998
  • Pipeline finished with Failed
    3 months ago
    Total: 497s
    #417046
  • Pipeline finished with Canceled
    3 months ago
    Total: 87s
    #417068
  • Pipeline finished with Success
    3 months ago
    Total: 508s
    #417072
  • Comment 3 months ago →
    🇺🇸United States phenaproxima Massachusetts

    Turns out the tests were only failing because we weren't always passing the filters to load() in ProjectBrowser.svelte. That makes sense -- we probably always want to pass them explicitly.

  • Pipeline finished with Skipped
    3 months ago
    #417086
  • First commit to issue fork.
  • Comment 3 months ago →
    System Message
  • Comment 3 months ago →
    🇺🇸United States chrisfromredfin Portland, Maine

    I think with automated tests passing for a known reason, and the code changes being grokable, and me manual testing, I think this one looks good!

  • Comment 2 months ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

  • Pipeline finished with Canceled
    about 2 months ago
    Total: 684s
    #433471
  • Pipeline finished with Success
    about 2 months ago
    Total: 1127s
    #433478
  • Pipeline finished with Success
    about 2 months ago
    Total: 1078s
    #433681
  • Pipeline finished with Skipped
    about 2 months ago
    #433710
contrib.social Blog FAQ Discussions
Production build 0.71.5 2024