[DataGrid] Fix overflow of maximum page #1583

oliviertassinari · 2021-05-07T14:15:27Z

A continuation of #1571

dtassone · 2021-05-07T14:17:27Z

packages/grid/_modules_/grid/hooks/features/pagination/gridPaginationReducer.ts

@@ -88,7 +88,7 @@ export const setGridRowCountStateUpdate = (state, payload): GridPaginationState
      ...state,
      pageCount: newPageCount,
      rowCount: totalRowCount,
-      page: state.page > newPageCount ? newPageCount - 1 : state.page,
+      page: Math.min(state.page, newPageCount - 1),


How is that different 🤔

min is equivalent to:

Suggested change

page: Math.min(state.page, newPageCount - 1),

page: state.page > newPageCount -1 ? newPageCount - 1 : state.page,,

ah yes of course!
IMO , it's easier and more common to read page: state.page > newPageCount -1 ? newPageCount - 1 : state.page,

I have used min because it makes it impossible for this error to happen, and since it went through two reviews unnoticed, I thought that we might as well enforce it. No real preference.

oliviertassinari · 2021-05-07T14:28:41Z

Arf, it doesn't seem to work, there is a regression in Argos-CI, with one of the test case

oliviertassinari · 2021-05-07T14:36:30Z

It seems to be a regression from #1571 that the snapshot test catches by chance.

dtassone · 2021-05-07T14:38:36Z

It seems to be a regression from #1571 that the snapshot test catches by chance.

the check was green in the other PR 🤔

oliviertassinari · 2021-05-07T14:45:55Z

the check was green in the other PR 🤔

I have added a proper test case for it in 4db548b (snapshots test are unreliable for this). It passes on HEAD and fails on these two PRs.

dtassone · 2021-05-07T14:51:31Z

The issue is that the page value is out of range when applying this filter model

dtassone · 2021-05-07T14:57:35Z

The issue is that the page value is out of range when applying this filter model

Check https://codesandbox.io/s/material-demo-forked-87xex
It's an issue with the page not the filtering

ZeeshanTamboli · 2021-05-07T17:18:30Z

Thanks Olivier for fix. It's all happening I think because page starts from zero index and the new pageCount is calculated as Math.ceil(rowCount, pageSize) which rounds up to largest integer. So in case of zero based decimals eg: 0.04 it's 1. So newPageCount always needs to be decremented by 1. (newPageCount - 1) like you did. I missed this one case.
Should we reconsider if Math.ceil is correct and thus instead avoid decrementing newPageCount by 1?
https://github.com/mui-org/material-ui-x/blob/00526d79d3cba481021afefe2a2a57591aa9283a/packages/grid/_modules_/grid/hooks/features/pagination/gridPaginationReducer.ts#L56

The issue is that the page value is out of range when applying this filter model

Check https://codesandbox.io/s/material-demo-forked-87xex
It's an issue with the page not the filtering

Won't filtered data will always be less than the total initial count of rows. So page range won't exceed, right? Unless user adds a page out of range which will trigger the warning.

ZeeshanTamboli · 2021-05-07T17:20:21Z

Test case failures looks related to auto controlling of page size.

oliviertassinari · 2021-05-07T18:00:58Z

Test case failures looks related to auto controlling of page size.

@ZeeshanTamboli True, I have a fix for it.

Regarding the issue found by @dtassone in #1583 (comment), I think that we can open a new GitHub issue for it, it looks wrong as well but can be solved independently.

oliviertassinari · 2021-05-07T20:23:00Z

Alright, It seems to work correctly now. I went on refactoring the hook. I didn't need it to do it here, but while I was as it, why not.

ZeeshanTamboli · 2021-05-10T12:20:26Z

Doesn't this PR fully fix #1571 which we are including in alpha-28? So why are we are not considering this for alpha-28?

oliviertassinari · 2021-05-10T13:22:54Z

@ZeeshanTamboli We ship at a fixed frequency, as long as there are no blockers on HEAD. #1571 is already an improvement by solving the case where the active page is above the last page + 1. Solving the +1 offset (#1583) is another improvement that can come independently.

dtassone · 2021-05-10T13:28:21Z

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts


-const PAGINATION_STATE_ID = 'pagination';
+function computeState(state) {


function computeState(state: GridPaginationState): GridPaginationState {

I would also rename computeState to getUpdatedState

I have updated the name of the function to better describe the use case. It's meant to compute derivative states. In theory, we could remove them from the state, it could be done inside a selector but I didn't want to refactor too much 🤷‍♂️

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts

dtassone · 2021-05-11T10:47:58Z

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts

-      );
+      logger.debug(`Setting page size to ${pageSize}`);
+
+      setGridState((oldState) => ({


I would rename to oldState to state

In the codebase, I can find:

setGridState((state x29

setGridState((oldState x16

setGridState((previousState x1

setGridState((p x1

OK, it would be great to do a follow-up to clean this up

setGridState((state ... x29 👍

updated with the review, see the last 2 commits

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts

oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! labels May 7, 2021

dtassone reviewed May 7, 2021

View reviewed changes

dtassone approved these changes May 7, 2021

View reviewed changes

oliviertassinari force-pushed the improve-#1571 branch from f1fe3f0 to 4db548b Compare May 7, 2021 14:46

oliviertassinari force-pushed the improve-#1571 branch 2 times, most recently from dbe597e to 51d5526 Compare May 7, 2021 20:01

oliviertassinari requested a review from dtassone May 7, 2021 20:23

oliviertassinari mentioned this pull request May 10, 2021

[DataGrid] rowCount prop updates are not taken into account #1614

Closed

dtassone previously requested changes May 11, 2021

View reviewed changes

dtassone reviewed May 11, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts Outdated Show resolved Hide resolved

dtassone reviewed May 11, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts Outdated Show resolved Hide resolved

dtassone reviewed May 11, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts Outdated Show resolved Hide resolved

dtassone reviewed May 11, 2021

View reviewed changes

oliviertassinari mentioned this pull request May 14, 2021

[DataGrid] Fix resulted filter data shows blank screen during pagination #1571

Merged

oliviertassinari added 3 commits May 16, 2021 17:02

only 2 columns is enough

56a14e8

update test case to be failing

0fb66d6

fix test case

cb70e77

oliviertassinari added 7 commits May 16, 2021 17:02

high resolution assertion

13d4dae

add test case

379e9cd

fix test case

02a39b6

drop reducer

8cd4428

fix regression

517195f

make the button easier to spot

bace64b

apply review

66bc37f

oliviertassinari force-pushed the improve-#1571 branch from c1edb7a to 66bc37f Compare May 16, 2021 15:40

keep it simple

76d2dda

oliviertassinari requested a review from dtassone May 16, 2021 16:25

dtassone reviewed May 17, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts Outdated Show resolved Hide resolved

dtassone reviewed May 17, 2021

View reviewed changes

packages/grid/_modules_/grid/hooks/features/pagination/useGridPagination.ts Outdated Show resolved Hide resolved

latest code review changes

3c88853

dtassone approved these changes May 18, 2021

View reviewed changes

dtassone merged commit 7baf6b8 into mui:master May 18, 2021

oliviertassinari deleted the improve-#1571 branch May 18, 2021 11:07

dtassone mentioned this pull request May 18, 2021

[docs] Fix docs demo #1691

Merged

oliviertassinari mentioned this pull request Jun 2, 2021

[DataGrid] Setting page with ApiRef in useEffect does not work anymore #1815

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Fix overflow of maximum page #1583

[DataGrid] Fix overflow of maximum page #1583

oliviertassinari commented May 7, 2021

dtassone May 7, 2021

oliviertassinari May 7, 2021

dtassone May 7, 2021

oliviertassinari May 7, 2021 •

edited

Loading

oliviertassinari commented May 7, 2021

oliviertassinari commented May 7, 2021

dtassone commented May 7, 2021

oliviertassinari commented May 7, 2021 •

edited

Loading

dtassone commented May 7, 2021

dtassone commented May 7, 2021 •

edited

Loading

ZeeshanTamboli commented May 7, 2021 •

edited

Loading

ZeeshanTamboli commented May 7, 2021

oliviertassinari commented May 7, 2021 •

edited

Loading

oliviertassinari commented May 7, 2021

ZeeshanTamboli commented May 10, 2021 •

edited

Loading

oliviertassinari commented May 10, 2021

dtassone May 10, 2021

dtassone May 11, 2021

oliviertassinari May 16, 2021 •

edited

Loading

dtassone May 11, 2021

oliviertassinari May 16, 2021 •

edited

Loading

dtassone May 17, 2021

	page: Math.min(state.page, newPageCount - 1),
	page: state.page > newPageCount -1 ? newPageCount - 1 : state.page,,


		const PAGINATION_STATE_ID = 'pagination';
		function computeState(state) {

[DataGrid] Fix overflow of maximum page #1583

[DataGrid] Fix overflow of maximum page #1583

Conversation

oliviertassinari commented May 7, 2021

dtassone May 7, 2021

Choose a reason for hiding this comment

oliviertassinari May 7, 2021

Choose a reason for hiding this comment

dtassone May 7, 2021

Choose a reason for hiding this comment

oliviertassinari May 7, 2021 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented May 7, 2021

oliviertassinari commented May 7, 2021

dtassone commented May 7, 2021

oliviertassinari commented May 7, 2021 • edited Loading

dtassone commented May 7, 2021

dtassone commented May 7, 2021 • edited Loading

ZeeshanTamboli commented May 7, 2021 • edited Loading

ZeeshanTamboli commented May 7, 2021

oliviertassinari commented May 7, 2021 • edited Loading

oliviertassinari commented May 7, 2021

ZeeshanTamboli commented May 10, 2021 • edited Loading

oliviertassinari commented May 10, 2021

dtassone May 10, 2021

Choose a reason for hiding this comment

dtassone May 11, 2021

Choose a reason for hiding this comment

oliviertassinari May 16, 2021 • edited Loading

Choose a reason for hiding this comment

dtassone May 11, 2021

Choose a reason for hiding this comment

oliviertassinari May 16, 2021 • edited Loading

Choose a reason for hiding this comment

dtassone May 17, 2021

Choose a reason for hiding this comment

oliviertassinari May 7, 2021 •

edited

Loading

oliviertassinari commented May 7, 2021 •

edited

Loading

dtassone commented May 7, 2021 •

edited

Loading

ZeeshanTamboli commented May 7, 2021 •

edited

Loading

oliviertassinari commented May 7, 2021 •

edited

Loading

ZeeshanTamboli commented May 10, 2021 •

edited

Loading

oliviertassinari May 16, 2021 •

edited

Loading

oliviertassinari May 16, 2021 •

edited

Loading