-
Notifications
You must be signed in to change notification settings - Fork 4
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
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.
This looks pretty good. There is one potential sneaky bug related to restricting filterOps
when a new filterKey
is selected.
const selectedFieldOperators = | ||
selectedField.operators !== undefined | ||
? operators.filter((element) => { | ||
return selectedField.operators.includes(element.value); | ||
}) | ||
: operators; |
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.
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;
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.
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(""); | ||
} |
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.
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.
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.
Good catch! Thank you, totally missed this one.
@tdilauro I think this is ready for another review. I added another test and addressed both your code review comments. |
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.
Looks good! 🎉
Description
Add a new filter to the list search that allows filtering on
fiction
andnonfiction
. If the new filter is selected, the input is replaced with a select list, so that the values you can enter are restricted tofiction
andnonfiction
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: