Problem/Motivation

#3464450: Certain filters should only be shown if they are actually defined introduces a new PHP API for the source plugins to define which filters they will respect. The sources only define the filters as a data type ("boolean", "multiple choice", etc.), but have absolutely no input or opinion about how they are rendered.

Right now, the frontend does not use the source-defined filters for rendering, nor does it really interact with them, in any way beyond using them as indicators of whether the pre-existing hard-coded filters should be displayed.

The next step is to actually render the filters defined by the source, by creating new Svelte components which can render them cleanly.

Proposed resolution

There are currently two filter types definable by the backend - BooleanFilter and MultipleChoiceFilter. Let's create some corresponding front-end Svelte components (<BooleanFilter> and <MultipleChoiceFilter>) which can render these filters and handle user interaction, and then use those instead of the hard-coded filters we currently show.

This is another case where we don't need explicit test coverage, as long as all the tests in HEAD pass. There should be no visible difference, or interactive difference, between what's in HEAD and the new components. All we're changing is the way they internally operate.

Issue fork project_browser-3470540

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

kunal.sachdev’s picture

kunal.sachdev commented
Status: Postponed » Active

Now that #3464450: Certain filters should only be shown if they are actually defined is fixed.

kunal.sachdev’s picture

kunal.sachdev commented
Title: [PP-1] Create some front-end components to render filters defined by the source » Create some front-end components to render filters defined by the source

kunal.sachdev opened merge request !574

kunal.sachdev’s picture

kunal.sachdev commented
Status: Active » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

I think this looks quite close to what I envisioned - great job!

The only thing that I object to here is the changes to BooleanFilter. I think that passing a set of options to it misunderstands the nature of a boolean filter.

A boolean filter really should only be a single checkbox, semantically speaking. I understand that Project Browser has shown them, previously, as option sets -- that's a strange decision, but it's one we could account for differently, rather than changing BooleanFilter to work like a set of options.

So, I'm kicking this back for that, but once that's done, we can work on resolving the test failures.

kunal.sachdev’s picture

kunal.sachdev commented

The current test failure is in tests/src/Nightwatch/Tests/keyboardTest.js in which there is a part where we are testing that when we are on the categories filter and we click tab then it shifts to next filter drop down i.e security coverage. This test is failing now because in this MR we are rendering the filters as it is which is defined from the backend and not hardcoding them. The previous hardcoding was done in a way such that the filters were displayed in the order :- categories -> security coverage -> maintenance status -> development status but now we accidentally in the backend have the filters defined in the order :- categories ->maintenance status -> security coverage -> development status. For now to fix the failure we can fix the order of the defined filters in backend but I think the major problem is that somewhere in the code we have hard coded the tab index for each filter which needs to be fixed (maybe in a new issue).

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

Virtually no complaints here. Amazing work!

I only would like to see a couple of follow-ups filed (one of which seems ambiguous), and using assertSame() instead of assertEquals() in most situations. Other than that, this just needs a little manual testing and then it's good to go.

kunal.sachdev’s picture

kunal.sachdev commented
Status: Needs work » Needs review
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Reviewed & tested by the community

This code looks good to me. I'm RTBCing it and sending it to @chrisfromredfin for manual testing and hopefully a merge.

chrisfromredfin made their first commit to this issue’s fork.

chrisfromredfin’s picture

chrisfromredfin
he/him/his
Primary language English
Location Portland, Maine
commented
Status: Reviewed & tested by the community » Fixed

Manually tested and re-reviewed. This is a big leap forward to moving out of proof-of-concept land and into "this is a real thing" land.

I would love to someday see a meta about revisiting all our tests and doing things like moving to assertSame(), but I may leave that for a core gate.

Status: Fixed » Closed (fixed)

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