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

Jump to bottom

Add support for nested filtering in useListData #6124

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+87 −5
Open

Add support for nested filtering in useListData #6124

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c5cd1c2
Add support for nested filtering in useListData
gijsroge Mar 29, 2024
ae53adf
Merge branch 'main' into main
snowystinger Apr 2, 2024
e14ddc5
Update test to show multiple valid sections
snowystinger Apr 2, 2024
56a1037
Merge branch 'adobe:main' into main
gijsroge May 2, 2024
444c1f3
Use configurable filterKey to filter nested sections
gijsroge May 2, 2024
fdc0b0e
Merge branch 'adobe:main' into main
gijsroge May 5, 2024
5acde87
Merge branch 'main' into main
snowystinger May 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 21 additions & 2 deletions packages/@react-stately/data/src/useListData.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,28 @@ export function useListData<T>(options: ListOptions<T>): ListData<T> {
filterText: initialFilterText
});

const filterItems = (items: T[], filterText: string): T[] => {
return items.reduce((acc: T[], item: any) => {
if (item.children) {
Copy link
Member

Choose a reason for hiding this comment

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

This is assuming something about the data structure of an item that isn't specified in the types. What if section objects use a different property name for their children or they come from a different data source entirely? I don't think useListData should be prescriptive about the objects that get passed into it.

If your items have children maybe useTreeData would be more appropriate?

Copy link
Author

@gijsroge gijsroge Apr 3, 2024 •
edited

Choose a reason for hiding this comment

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

Yeah I see what you mean. The reason for this PR was mostly to expand support for Listbox filtering (nested items), AFAIK listbox does not support useTreeData and useTreeData does not support filtering out of the box?

What would be your ideal scenario to add support for this? Maybe a more generic way of filtering that does not depend on a specific property?

Maybe something like this?

useListData<T>({
  initialItems: items,
  filterKeys: ['children'],
  filter: (item: any, text) => {
    return contains(item.name, text)
  }
})

Copy link
Member

Choose a reason for hiding this comment

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

So listbox does actually support being passed items from useTreeData, but you are right about useTreeData not supporting filtering out of the box. I've raised a question to the team about whether we'd prefer to commit to having nested item filter support supported useListData (which would then mean we'd need to update the rest of the useListData methods to work for nested data) or if we'd instead add filtering to useTreeData.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for your patience. We had an internal discussion and decided we'd like to add filtering to useTreeData instead of having useListData be another tree implementation.

Would you like to have a go at that in this PR?

Copy link
Author

@gijsroge gijsroge May 13, 2024 •
edited

Choose a reason for hiding this comment

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

@snowystinger understood, feels better to do it in useTreeData indeed. I will take a stab at it. Do I start a new PR for this?

Copy link
Member

Choose a reason for hiding this comment

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

either one, whatever you're more comfortable with. If you use this one, we'll just rename it

if (filter(item, filterText)) {
acc.push(item);
snowystinger marked this conversation as resolved.
Show resolved Hide resolved
} else {
const children = filterItems(item.children, filterText);
if (children.length > 0) {
acc.push({...item, children});
}
}
} else if (filter(item, filterText)) {
acc.push(item);
}
return acc;
}, []);
};

let filteredItems = useMemo(
() => filter ? state.items.filter(item => filter(item, state.filterText)) : state.items,
[state.items, state.filterText, filter]);
() => (filter ? filterItems(state.items, state.filterText) : state.items),
[state.items, state.filterText, filter]
);

return {
...state,
Expand Down
27 changes: 27 additions & 0 deletions packages/@react-stately/data/test/useListData.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,24 @@ const many = [
{name: 'Eleven'}
];

const grouped = [
{
name: 'One',
children: [
{name: 'Child 1'},
{name: 'Child 2'}
]
},
{name: 'Two', children: [{name: 'Child 0'}]},
{
name: 'Three',
children: [
{name: 'Child 1'}
]
}
];


let getKey = (item) => item.name;
let filter = (item, text) => item.name.includes(text);

Expand Down Expand Up @@ -801,4 +819,13 @@ describe('useListData', function () {
expect(result.current.items).toHaveLength(1);
expect(result.current.items[0]).toEqual({name: 'David'});
});

it('should support filtering items across sections', function () {
let {result} = renderHook(() => useListData({initialItems: grouped, getKey, filter, initialFilterText: 'Child 1'}));

expect(result.current.items).toEqual([
{name: 'One', children: [{name: 'Child 1'}]},
{name: 'Three', children: [{name: 'Child 1'}]}
]);
});
});