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.

Sign up for GitHub

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

Add fiction list filter (PP-1110) #106

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9599f2c
Add fiction list filter
jonathangreen Mar 27, 2024
2aa2eb4
Some cleanup
jonathangreen Mar 27, 2024
078c364
css tweaks
jonathangreen Mar 27, 2024
c4fb0be
Add some tests
jonathangreen Mar 27, 2024
0e696bc
Code review feedback
jonathangreen Mar 28, 2024
1645480
Update filter operator as well, if needed. Add an additional test for…
jonathangreen Mar 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/components/AdvancedSearchBuilder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ export const fields = [
{ value: "audience", label: "audience" },
{ value: "author", label: "author" },
{ value: "title", label: "title" },
{
value: "fiction",
label: "fiction",
options: ["fiction", "nonfiction"],
operators: ["eq"],
},
];

export const operators = [
Expand Down
35 changes: 32 additions & 3 deletions src/components/AdvancedSearchFilterInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ export default function AdvancedSearchFilterInput({

const handleKeyChange = (value) => {
setFilterKey(value);
const selected = fields.find((field) => field.value === value);

// Set the value to either the default option, or blank when changing the filter type
if (selected.options && selected.options.length) {
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.

};

const handleOpChange = () => {
Expand Down Expand Up @@ -64,6 +72,17 @@ export default function AdvancedSearchFilterInput({
};

const selectedField = fields.find((field) => field.value === filterKey);
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.


const selectedFieldOptions = selectedField.options ?? [];
const selectedFieldElementType = selectedFieldOptions.length
? "select"
: "input";

return (
<form className="advanced-search-filter-input">
Expand All @@ -89,8 +108,9 @@ export default function AdvancedSearchFilterInput({
onChange={handleOpChange}
ref={opSelect}
value={filterOp}
className="filter-operator"
>
{operators.map(({ value, label }) => (
{selectedFieldOperators.map(({ value, label }) => (
<option
aria-selected={value === filterOp}
key={value}
Expand All @@ -104,14 +124,23 @@ export default function AdvancedSearchFilterInput({
<EditableInput
aria-label="filter value"
description={selectedField.helpText}
elementType="input"
elementType={selectedFieldElementType}
type="text"
optionalText={false}
placeholder={selectedField.placeholder}
ref={valueInput}
value={filterValue}
onChange={handleValueChange}
/>
className="filter-value"
>
{selectedFieldOptions.length === 0
? null
: selectedFieldOptions.map((value) => (
<option key={value} value={value}>
{value}
</option>
))}
</EditableInput>

<Button
className="inverted inline"
Expand Down
49 changes: 48 additions & 1 deletion src/components/__tests__/AdvancedSearchFilterInput-test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,59 @@ describe("AdvancedSearchFilterInput", () => {
});
});

it("should restrict the operator selection, if the filter requests it", () => {
const fictionRadioButton = wrapper.find(
'input[type="radio"][value="fiction"]'
);
fictionRadioButton.getDOMNode().checked = true;
fictionRadioButton.simulate("change");

const options = wrapper.find(".filter-operator select > option");
const fictionField = fields.find((element) => {
return element.value === "fiction";
});

expect(options).to.have.length(fictionField.operators.length);
});

it("should render a text input for value entry", () => {
const valueInput = wrapper.find('input[type="text"]');
const valueSelect = wrapper.find(".filter-value select");
const valueInput = wrapper.find('.filter-value input[type="text"]');

expect(valueSelect).to.have.length(0);
expect(valueInput).to.have.length(1);
});

it("should render a select for value selection, if the filter requests it", () => {
const fictionRadioButton = wrapper.find(
'input[type="radio"][value="fiction"]'
);
fictionRadioButton.getDOMNode().checked = true;
fictionRadioButton.simulate("change");

const valueSelect = wrapper.find(".filter-value select");
const valueInput = wrapper.find('.filter-value input[type="text"]');

expect(valueSelect).to.have.length(1);
expect(valueInput).to.have.length(0);
});

it("should clear the current value when changing the filter", () => {
const valueInput = wrapper.find(".filter-value input");

valueInput.getDOMNode().value = "ABC";
valueInput.simulate("change");

const filterRadioButton = wrapper.find(
'input[type="radio"][value="data_source"]'
);

filterRadioButton.getDOMNode().checked = true;
filterRadioButton.simulate("change");

expect(valueInput.getDOMNode().value).to.equal("");
});

it("should render an Add Filter button", () => {
const button = wrapper.find("button");

Expand Down
12 changes: 10 additions & 2 deletions src/stylesheets/advanced_search.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,16 @@
}
}

input[type="text"] {
width: 40em;
.filter-value {
input[type="text"],select {
width: 40em;
}
}

.filter-operator {
select {
width: 15em;
}
}

.filter-key-options {
Expand Down
Loading