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 <InfiniteList> and <InfinitePagination> components #8781

Merged
merged 29 commits into from
Apr 5, 2023
Merged

Conversation

fzaninotto
Copy link
Member

@fzaninotto fzaninotto commented Mar 29, 2023

<InfiniteList> is a drop-in replacement for <List> that leverages useInfiniteGetList() to allow infinite pagination with <SimpleList> and <Datagrid>

  • Add useInfiniteListController hook
  • Add <InfiniteListBase> component
  • Add <InfiniteList> component
  • Add <InfinitePagination> component
  • Fix data provider mutation hooks don't update the infinite query cache
  • Add story to demonstrate it
  • Use it in simple example
  • Find a way to display the total number of results
  • Add tests (must be e2e because jsdom doesn't have an IntersectionObserver)
  • Add documentation

Sample usage

const BookList = () => (
    <InfiniteList>
        <SimpleList primaryText={record => record.title} />
    </InfiniteList>
);
Enregistrement.de.l.ecran.2023-03-29.a.19.33.58.mov

@@ -80,7 +79,7 @@ export const useInfiniteGetList = <RecordType extends RaRecord = any>(
Error,
GetInfiniteListResult<RecordType>
>(
[resource, 'getList', { pagination, sort, filter, meta }],
[resource, 'getInfiniteList', { pagination, sort, filter, meta }],
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary because otherwise the normal and infinite list queries share the same cache, which leads to errors because they don't have the same structure.

This means that all the dataProvider hooks that optimistically update the getList query cache must also do it for the getInfiniteList.

export const useSidebarState = (): useSidebarStateResult =>
useStore<boolean>('sidebar.open', true);
export const useSidebarState = (): useSidebarStateResult => {
const isXSmall = useMediaQuery<Theme>(
Copy link
Member Author

Choose a reason for hiding this comment

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

The sidebar was open by default on Mobile. It was not user-friendly, but most importantly it prevented e2e testing on mobile, as every test had to start by closing the menu.

Copy link
Contributor

@djhi djhi left a comment

Choose a reason for hiding this comment

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

Awesome!

</Admin>
);

export const PaginationClassic = () => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a story showing how to load the next page using a single Load more button would be more relevant. Even better, I think we should provide such button by default in addition to the InfinitePagination component.
InfinitePaginationButton ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote this story to show that the default pagination callbacks still work even for the infinite list. I think we should keep it.

For the "Load more" pagination, it's a good idea. I created a story and a doc entry, but just like the component that displays the count, I'm not sure it's a standard solution, so I prefer not to publish a component for now.

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

👏

docs/InfiniteList.md Outdated Show resolved Hide resolved
Comment on lines +98 to +115
const CustomPagination = () => {
const { total } = useListContext();
return (
<>
<InfinitePagination />
{total > 0 && (
<Box position="sticky" bottom={0} textAlign="center">
<Card
elevation={2}
sx={{ px: 2, py: 1, mb: 1, display: 'inline-block' }}
>
<Typography variant="body2">{total} results</Typography>
</Card>
</Box>
)}
</>
);
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not make a component out of this too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I don't think this is a general solution, it's just an example. People will want to put the number of results on top of the list, in a datagrid footer, in the action bar, etc... There is no standard for that UI.

docs/navigation.html Show resolved Hide resolved
@slax57
Copy link
Contributor

slax57 commented Apr 4, 2023

Not sure if this should qualify as a bug or not, but when your screen is already tall enough to display more than 2 pages of results, then the 3rd page is never fetched (only the second page is fetched).

See attached screen capture.

Peek.04-04-2023.16-13.webm

@fzaninotto
Copy link
Member Author

Not sure if this should qualify as a bug or not, but when your screen is already tall enough to display more than 2 pages of results, then the 3rd page is never fetched (only the second page is fetched).

See attached screen capture.

That's absolutely a bug. Good catch!

@fzaninotto
Copy link
Member Author

Should be OK now!

},
});

const Admin = ({ children, dataProvider, layout }: any) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you wrap provide a new queryClient, memoryHistory and store to each story? Otherwise, visiting filtered stories make the others (pagination related for instance) weird as the filters are already stored and applied

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope :)

queryClient, memoryHistory and store

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessary in my tests: the stories don't share a state anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

Checked and confirmed

@djhi
Copy link
Contributor

djhi commented Apr 5, 2023

It needs a rebase though

@fzaninotto
Copy link
Member Author

rebased

@djhi djhi merged commit 0e1a1ce into next Apr 5, 2023
@djhi djhi deleted the infinite-list branch April 5, 2023 14:26
@fzaninotto fzaninotto added this to the 4.10.0 milestone Apr 6, 2023
@fzaninotto fzaninotto changed the title Infinite list Add <InfiniteList> and <InfinitePagination> components Apr 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants