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

feat(filter): adds ability to match only specific filter data properties #9541

Merged
merged 5 commits into from
Jun 14, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
8 changes: 8 additions & 0 deletions packages/calcite-components/src/components.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1805,6 +1805,10 @@ export namespace Components {
* Defines the items to filter. The component uses the values as the starting point, and returns items that contain the string entered in the input, using a partial match and recursive search. This property is needed to conduct filtering.
*/
"items": object[];
/**
* Specifies the fields to match against when filtering.
*/
"matchFields": string[];
/**
* Use this property to override individual strings used by the component.
*/
Expand Down Expand Up @@ -9608,6 +9612,10 @@ declare namespace LocalJSX {
* Defines the items to filter. The component uses the values as the starting point, and returns items that contain the string entered in the input, using a partial match and recursive search. This property is needed to conduct filtering.
*/
"items"?: object[];
/**
* Specifies the fields to match against when filtering.
*/
"matchFields"?: string[];
/**
* Use this property to override individual strings used by the component.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,14 @@ describe("calcite-filter", () => {
await page.waitForTimeout(DEBOUNCE_TIMEOUT);
assertMatchingItems(await filter.getProperty("filteredItems"), ["harry"]);
});

it("should return no matching values", async () => {
const filter = await page.find("calcite-filter");
filter.setProperty("matchFields", ["description"]);
await page.waitForChanges();
await page.waitForTimeout(DEBOUNCE_TIMEOUT);
assertMatchingItems(await filter.getProperty("filteredItems"), []);
});
});

describe("filter method", () => {
Expand Down
15 changes: 13 additions & 2 deletions packages/calcite-components/src/components/filter/filter.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ export class Filter
*/
@Prop({ mutable: true }) filteredItems: object[] = [];

/**
* Specifies the fields to match against when filtering.
driskull marked this conversation as resolved.
Show resolved Hide resolved
*/
@Prop() matchFields: string[];

@Watch("matchFields")
matchFieldsHandler(): void {
this.filterDebounced(this.value);
}

/**
* Specifies placeholder text for the input element.
*/
Expand Down Expand Up @@ -159,7 +169,7 @@ export class Filter
async componentWillLoad(): Promise<void> {
setUpLoadableComponent(this);
if (this.items.length) {
this.updateFiltered(filter(this.items, this.value));
this.updateFiltered(filter(this.items, this.value, this.matchFields));
}
await setUpMessages(this);
}
Expand Down Expand Up @@ -223,7 +233,8 @@ export class Filter

private filterDebounced = debounce(
(value: string, emit = false, onFilter?: () => void): void =>
this.items.length && this.updateFiltered(filter(this.items, value), emit, onFilter),
this.items.length &&
this.updateFiltered(filter(this.items, value, this.matchFields), emit, onFilter),
DEBOUNCE_TIMEOUT,
);

Expand Down
6 changes: 3 additions & 3 deletions packages/calcite-components/src/components/list/list.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -365,12 +365,12 @@ describe("calcite-list", () => {
const list = await page.find("calcite-list");
await page.waitForTimeout(listDebounceTimeout);

expect(await list.getProperty("filteredItems")).toHaveLength(3);
expect(await list.getProperty("filteredData")).toHaveLength(3);
expect(await list.getProperty("filteredItems")).toHaveLength(2);
driskull marked this conversation as resolved.
Show resolved Hide resolved
expect(await list.getProperty("filteredData")).toHaveLength(2);

const visibleItems = await page.findAll("calcite-list-item:not([filter-hidden])");

expect(visibleItems.map((item) => item.id)).toEqual(["label-match", "description-match", "value-match"]);
expect(visibleItems.map((item) => item.id)).toEqual(["label-match", "description-match"]);
});

it("should support shift click to select multiple items", async () => {
Expand Down
1 change: 1 addition & 0 deletions packages/calcite-components/src/components/list/list.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,7 @@ export class List
aria-label={filterPlaceholder}
disabled={disabled}
items={dataForFilter}
matchFields={["label", "description", "metadata"]}
Copy link
Member

Choose a reason for hiding this comment

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

I think this also needs to include value. Otherwise, we're changing existing filtering behavior.

Copy link
Member Author

@driskull driskull Jun 12, 2024

Choose a reason for hiding this comment

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

I think we should consider this a bug. It was never intended to search by value. Value is more of a unique identifier and not content like label, description and metadata are.

Copy link
Member Author

Choose a reason for hiding this comment

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

For filtering, we should only be searching content. I wouldn't consider value to be content. I think we should not search it by default for anyone using a list with a filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about I move the list changes to a separate followup PR with the title fix(list): only filter by list item content? @jcfranco

Copy link
Member

Choose a reason for hiding this comment

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

I think we should consider this a bug. It was never intended to search by value. Value is more of a unique identifier and not content like label, description and metadata are.

I disagree with this. The filtering has always included value props by default. Could we introduce a matchFields prop to all filtering components, allowing users to omit value if they prefer?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jcfranco I reverted the list changes in this PR. Can we discuss it as part of #9569? I can create a separate issue for adding matchFields to list if we decide to do that.

Is this PR good to merge?

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha. Will give this another look.

onCalciteFilterChange={this.handleFilterChange}
placeholder={filterPlaceholder}
ref={this.setFilterEl}
Expand Down
66 changes: 66 additions & 0 deletions packages/calcite-components/src/utils/filter.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import { filter } from "./filter";

describe("filter function", () => {
it("warns and returns empty array for empty data", () => {
const originalWarn = console.warn;
console.warn = jest.fn();
driskull marked this conversation as resolved.
Show resolved Hide resolved
const result = filter([], "test");
expect(console.warn).toHaveBeenCalled();
expect(result).toEqual([]);
console.warn = originalWarn;
});

it("returns empty array when no objects match", () => {
const data = [{ name: "John" }, { name: "Jane" }];
const result = filter(data, "Doe");
expect(result).toEqual([]);
});

it("returns matching objects", () => {
const data = [{ name: "John" }, { name: "Jane" }];
const result = filter(data, "Jane");
expect(result).toEqual([{ name: "Jane" }]);
});

it("considers only specified fields for matching", () => {
const data = [
{ name: "John", age: 30 },
{ name: "Jane", age: 25 },
];
const result = filter(data, "25", ["age"]);
expect(result).toEqual([{ name: "Jane", age: 25 }]);

const result2 = filter(data, "John", ["age"]);
expect(result2).toEqual([]);
});

it("ignores functions and null values in objects", () => {
driskull marked this conversation as resolved.
Show resolved Hide resolved
const data = [{ name: "John", action: () => {}, value: null }];
const result = filter(data, "John");
expect(result).toEqual([{ name: "John", action: expect.any(Function), value: null }]);
});

it("searches nested objects correctly", () => {
const data = [{ name: "John", details: { age: 30 } }];
const result = filter(data, "30");
expect(result).toEqual([{ name: "John", details: { age: 30 } }]);
});

it("searches arrays in objects correctly", () => {
driskull marked this conversation as resolved.
Show resolved Hide resolved
const data = [{ name: "John", tags: ["developer", "tester"] }];
const result = filter(data, "tester");
expect(result).toEqual([{ name: "John", tags: ["developer", "tester"] }]);
});

it("always includes objects with filterDisabled set to true", () => {
driskull marked this conversation as resolved.
Show resolved Hide resolved
const data = [{ name: "John" }, { name: "Jane", filterDisabled: true }];
const result = filter(data, "Doe");
expect(result).toEqual([{ name: "Jane", filterDisabled: true }]);
});

it("always includes objects with constant set to true", () => {
driskull marked this conversation as resolved.
Show resolved Hide resolved
const data = [{ name: "John" }, { name: "Jane", constant: true }];
const result = filter(data, "Doe");
expect(result).toEqual([{ name: "Jane", constant: true }]);
});
});
19 changes: 11 additions & 8 deletions packages/calcite-components/src/utils/filter.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { escapeRegExp, forIn } from "lodash-es";

export const filter = (data: Array<object>, value: string): Array<any> => {
export const filter = (data: Array<object>, value: string, matchFields?: string[]): Array<any> => {
const escapedValue = escapeRegExp(value);
const regex = new RegExp(escapedValue, "i");

Expand All @@ -9,16 +9,22 @@ export const filter = (data: Array<object>, value: string): Array<any> => {
The data argument should be an array of objects`);
}

const find = (input: object, RE: RegExp) => {
const find = (input: object, RE: RegExp, fields?: string[]) => {
if ((input as any)?.constant || (input as any)?.filterDisabled) {
return true;
}

let found = false;

forIn(input, (val) => {
forIn(input, (val: any, key) => {
driskull marked this conversation as resolved.
Show resolved Hide resolved
if (typeof val === "function" || val == null /* intentional == to catch undefined */) {
return;
}

if (fields && !fields.includes(key)) {
return;
}

if (Array.isArray(val) || (typeof val === "object" && val !== null)) {
if (find(val, RE)) {
found = true;
Expand All @@ -27,12 +33,9 @@ export const filter = (data: Array<object>, value: string): Array<any> => {
found = true;
}
});

return found;
};

const result = data.filter((item) => {
return find(item, regex);
});

return result;
return data.filter((item) => find(item, regex, matchFields));
driskull marked this conversation as resolved.
Show resolved Hide resolved
};
Loading