feat(react-grid): Ability to show all rows on a page via page size selector #150
Conversation
| @@ -1,8 +1,16 @@ | |||
| export const paginate = (rows, pageSize, page) => | |||
| rows.slice(pageSize * page, pageSize * (page + 1)); | |||
| const showAllPagesKey = 'all'; | |||
There was a problem hiding this comment.
Shouldn't we export this const and use it other files (where we reference the 'all' value)?
|
|
||
| export const ensurePageHeaders = (rows, pageSize) => { | ||
| const result = rows.slice(); | ||
| if (pageSize === showAllPagesKey) { |
There was a problem hiding this comment.
This expression is used four times in this file. May be create a helper function?
| @@ -1,8 +1,16 @@ | |||
| export const paginate = (rows, pageSize, page) => | |||
| rows.slice(pageSize * page, pageSize * (page + 1)); | |||
| const showAllPagesKey = 'all'; | |||
There was a problem hiding this comment.
Why have not you used upper case for this constant like we do in other cases (e.g. DEFAULT_HEIGHT or MINIMAL_COLUMN_WIDTH)?
I suggest renaming it in ALL_PAGES_KEY to make it look less alike a function.
| }; | ||
|
|
||
| export const pageSizeTitle = pageSize => ( | ||
| pageSize === showAllPagesKey ? 'All' : pageSize |
There was a problem hiding this comment.
Is it OK to have an inlined string here?
| @@ -16,7 +16,7 @@ totalCount | number | | Specifies the total rows count | |||
| currentPage | number | | Specifies the current page number | |||
| defaultCurrentPage | number | 0 | Specifies the initial current page for the uncontrolled mode | |||
| onCurrentPageChange | (currentPage: number) => void | | Handles current page changes | |||
| pageSize | number | | Specifies the page size | |||
| pageSize | number | | | Specifies the page size | |||
There was a problem hiding this comment.
Also, remove redundant | symbol
| @@ -1,8 +1,16 @@ | |||
| export const paginate = (rows, pageSize, page) => | |||
| rows.slice(pageSize * page, pageSize * (page + 1)); | |||
| const ALL_PAGES_TITLE = 'All'; | |||
There was a problem hiding this comment.
Used here export const pageSizeTitle = pageSize => pageSize || ALL_PAGES_TITLE;
There was a problem hiding this comment.
The pageSizeTitle export is strange, isn't it? It has a localizable string that can't be configured by a user.
|
|
||
| export const ensurePageHeaders = (rows, pageSize) => { | ||
| const result = rows.slice(); | ||
| if (!pageSize) { |
There was a problem hiding this comment.
Why could we not escape earlier? Slicing seems redundant
There was a problem hiding this comment.
Slicing is for array copying, isn't it?
There was a problem hiding this comment.
Yes, but this copy is needed only for processing listed below in this function. We can return original rows if processing is not needed.
| @@ -16,7 +16,7 @@ totalCount | number | | Specifies the total rows count | |||
| currentPage | number | | Specifies the current page number | |||
| defaultCurrentPage | number | 0 | Specifies the initial current page for the uncontrolled mode | |||
| onCurrentPageChange | (currentPage: number) => void | | Handles current page changes | |||
| pageSize | number | | | Specifies the page size | |||
| pageSize | number | | Specifies the page size | |||
There was a problem hiding this comment.
Maybe we should add info about 0 constant here?
There was a problem hiding this comment.
I've added it here packages/dx-react-grid/docs/guides/paging.md
There was a problem hiding this comment.
Yes, I saw it. I think that reference is a more suitable place for this. It is also good to have a note about 0 constant in guide and reference.
| @@ -14,6 +14,7 @@ Name | Type | Default | Description | |||
| -----|------|---------|------------ | |||
| pagerTemplate | (args: [PagerArgs](#pager-args)) => ReactElement | | A component that renders a pager based on the supplied parameters | |||
| allowedPageSizes | Array<number> | [] | Specifies the page sizes that can be selected at runtime | |||
| showAllText | string | | Specifies the text of the 'All' item of a page size selector | |||
There was a problem hiding this comment.
Add note about availability only BS3 and MUI plugin wrappers
| @@ -31,6 +32,7 @@ onCurrentPageChange | (page: number) => void | Changes the current page | |||
| pageSize | number | Specifies the page size | |||
| onPageSizeChange | (size: number) => void | Changes the page size | |||
| allowedPageSizes | Array<number> | Specifies the page sizes that can be selected at runtime | |||
| showAllText | string | Specifies the text of the 'All' item of a page size selector | |||
viterobk
requested review from
annnke,
AnnaTikh,
AnnaZelenova,
YanaOrlova and
dirk-pieterse
No description provided.