Improve the interface of the comprehensive pagination engine #1358
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Deployed to Cloudflare Pages
|
csillag
force-pushed
the
csillag/improve-table-pagination
branch
from
14361c3 to
45d5b8d
Compare
csillag
changed the title
csillag
force-pushed
the
csillag/improve-table-pagination
branch
2 times, most recently
from
b999f3d to
a21dfd7
Compare
csillag
force-pushed
the
csillag/improve-table-pagination
branch
4 times, most recently
from
ace62ea to
b6c3cda
Compare
- Add documentation - Rename things for clarity - Add support for transforming the data, besides filtering - Add support for also specifying filters as an array - Add a bunch of more status fields
csillag
force-pushed
the
csillag/improve-table-pagination
branch
from
b6c3cda to
5b3b562
Compare
lukaw3d
reviewed
May 23, 2024
| @@ -0,0 +1 @@ | |||
| Improve the comprehensive pagination engine | |||
Comment on lines
+10
to
13
| /** | ||
| * How should we call the query parameter (in the URL)? | ||
| */ | ||
| paramName: string |
There was a problem hiding this comment.
This is where selected page is stored?
Comment on lines
+23
to
+24
| * Please note that currently this engine doesn't handle when the data consumer requires data which is not | ||
| * part of the initial window on the server side. |
| * - transform() | ||
| * - filter | ||
| * - filters | ||
| * - order */ |
| /** | ||
| * Transformation to be applied after loading the data from the server, before presenting it to the data consumer component | ||
| * | ||
| * Can be used for ordering, aggregation, etc.D |
| type ClientSizePaginationParams<Item> = { | ||
| type Filter<Item> = (item: Item) => boolean | ||
|
|
||
| type ClientSizePaginationParams<Item, QueryResult extends List, ExtractedData = typeof undefined> = { |
There was a problem hiding this comment.
Suggested change
| type ClientSizePaginationParams<Item, QueryResult extends List, ExtractedData = typeof undefined> = { | |
| type ClientSitePaginationParams<Item, QueryResult extends List, ExtractedData = typeof undefined> = { |
| ? (queryResult[key] as Item[]) | ||
| : findListIn<QueryResult, Item>(queryResult) | ||
| selectedPageForClient: selectedClientPage, | ||
| paramsForServer: paramsForQuery, |
There was a problem hiding this comment.
rename both to paramsForServer?
| /** | ||
| * The data returned by a comprehensive pagination engine to the data consumer component | ||
| */ | ||
| export interface ComprehensivePaginatedResults<Item, ExtractedData = typeof undefined> { |
There was a problem hiding this comment.
what is typeof undefined type? is it string?
Comment on lines
+112
to
+114
| // From the server, we always want to load the first batch of data, with the provided (big) window. | ||
| // In theory, we could move this window as required, but currently this is not implemented. | ||
| const selectedServerPage = 1 |
There was a problem hiding this comment.
Is batch a different concept from window and page?
| const offset = (selectedClientPage - 1) * clientPageSize | ||
| const limit = clientPageSize | ||
| const dataWindow = filteredData ? filteredData.slice(offset, offset + limit) : undefined | ||
|
|
||
| // The control interface for the data consumer component (i.e. Table) | ||
| const tableProps: TablePaginationProps = { | ||
| selectedPage: selectedClientPage, | ||
| linkToPage, |
There was a problem hiding this comment.
Can you improve the comment below?
? // This correction is here to simulate the bogus behavior of server-side pagination
// Remove this when server-side pagination is fixed,
// and so the work-around in the pagination widget is fixed.
it doesn't say TODO, it doesn't say what's wrong, and repeats last line?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, we have two interfaces for pagination:
SimplePaginationEngine. One such engine is returned byuseSearchParamsPagination().ComprehensivePaginationEngine. We have two implementations for this:useComprehensiveSearchParamsPagination()anduseClientSidePagination()As you can guess from the name, the "Simple" engine is only used to provide parameters for queries, and driving the pagination ui. The "Comprehensive" engine has more responsibilities: it sits between the data source and the UI, completely encapsulating the data source, and therefore it is able to do various transformations (like sorting, filtering, etc).
Currently we only using this comprehensive engine on the token dashboard (plus on some feature branches, mostly where we want client-side pagination), so it's easy to introduce breaking API changes.
This PR polishes the interface of the comprehensive engine a little bit: