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

Add fiction list filter (PP-1110) #106

Merged
merged 6 commits into from
Mar 28, 2024
Merged

Conversation

jonathangreen
Copy link
Member

@jonathangreen jonathangreen commented Mar 27, 2024

Description

Add a new filter to the list search that allows filtering on fiction and nonfiction. If the new filter is selected, the input is replaced with a select list, so that the values you can enter are restricted to fiction and nonfiction since these are the only two valid values for the field.

Any feedback on this one would be great. It does accomplish what is asked in the ticket, and I think it works from the user perspective, in my testing at least. I only know enough to be dangerous about react though, so there may be better ways to accomplish what I'm doing here.

Motivation and Context

Would like to be able to filter custom lists by fiction status. See PP-1110.

How Has This Been Tested?

Tested against gorgon running the admin interface locally with the blackstone collection.

Checklist:

  • I have updated the documentation accordingly.
  • All new and existing tests passed.

@jonathangreen jonathangreen marked this pull request as ready for review March 27, 2024 19:09
Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

This looks pretty good. There is one potential sneaky bug related to restricting filterOps when a new filterKey is selected.

Comment on lines 75 to 80
const selectedFieldOperators =
selectedField.operators !== undefined
? operators.filter((element) => {
return selectedField.operators.includes(element.value);
})
: operators;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: This is fine and will work; but we could simplify it. Since there's only one statement in the filter function, we can drop the braces and the return. And because there's only one parameter, we can drop the parens. If we don't mind including null with the already-present undefined, we can also drop the comparison.

  const selectedFieldOperators = selectedField.operators
    ? operators.filter(op => selectedField.operators.includes(op.value))
    : operators;

Copy link
Member Author

Choose a reason for hiding this comment

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

That it much more readable, thanks! Eslint doesn't like dropping the braces around op, so I added those back in, but otherwise I made this change.

setFilterValue(selected.options[0]);
} else {
setFilterValue("");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There could be a problem here. This PR is introducing the potential for a reduced set of operators, so we'll also need to ensure that we've got a valid operator for the newly current field. Ideally, we'd only change the filterOp if the current one is not supported.

It would be useful to add a test simulating going from an op on a field with broader set ops to a field that doesn't support that op, then adding the filter, and finally verifying that the key, value, and op are as expected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Thank you, totally missed this one.

@jonathangreen jonathangreen requested a review from tdilauro March 28, 2024 14:07
@jonathangreen
Copy link
Member Author

@tdilauro I think this is ready for another review. I added another test and addressed both your code review comments.

Copy link
Contributor

@tdilauro tdilauro left a comment

Choose a reason for hiding this comment

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

Looks good! 🎉

@jonathangreen jonathangreen merged commit 01f7780 into main Mar 28, 2024
1 check passed
@jonathangreen jonathangreen deleted the feature/fiction-list-filter branch March 28, 2024 14:51
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.

2 participants