search

mui / mui-x

MUI X: Build complex and data-rich applications using a growing list of advanced React components, like the Data Grid, Date and Time Pickers, Charts, and more!
https://mui.com/x/
3.82k stars 1.14k forks source link

[data grid] onPaginationModelChange details property is not defined / useful in v7+ #12897

Open bjackson42 opened 3 weeks ago

bjackson42 commented 3 weeks ago

Steps to reproduce

Link to live example: (required)

v6 Pagination v7 Pagination

Steps:

  1. Go to v6 MUI DataGridPro pagination page and scroll down to Controlled pagination model
  2. Click "Expand Code"

  3. Update the onPaginationModel property to the following:

    onPaginationModelChange={(model, details) => {
  console.log(details);

  setPaginationModel(model)
}
}
  4. Open Developer Console (F12)
  5. Click the paging buttons on the example
  6. Take note that the console log says { reason: 'setPaginationModel' }
  7. Repeat these steps for v7
  8. Take note that the console log says { reason: undefined, api: {...} }

Current behavior

The details property is not defined when using the Grid pagination controls.

Expected behavior

The details property should have a string value for reason when using the Grid pagination controls.

Context

In v6 the DataGridPro would give a reason when using the Grid pagination controls. This made it possible to distinguish between a manual pagination model change versus a Grid pagination control model change.

An example of a manual pagination change is an Add button in the Grid toolbar that adds a new row. When pagination is enabled and a new row is added we should take the user to the page where the new row resides. If pageSize is 5 and a 6th row is added we should navigate the user to the second page with the new row. This was possible in v6 because the reason would be undefined and a developer could tell when a manual pagination model change had been made. Otherwise a reason is given and we honor the model suggested by onPaginationModelChange.

In v7 the details property value has changed from { reason: string } to { reason: undefined, api: {...} }. This caused a major breakage in our implementation and we have had to come up with a custom solution to get around the issue. To make the situation worse, there was no notice given about this change and I cannot find any record of this change.

Please advise, is there an alternative method? Should I just use the API even though the documentation suggests not to?

Your environment

npx @mui/envinfo ``` Chrome (latest) GitBash (latest) System: OS: Windows 11 10.0.22631 Binaries: Node: 20.9.0 - C:\Program Files\nodejs\node.EXE npm: 10.1.0 - C:\Program Files\nodejs\npm.CMD pnpm: Not Found Browsers: Chrome: Not Found Edge: Chromium (123.0.2420.97) npmPackages: @emotion/react: 11.11.4 => 11.11.4 @emotion/styled: 11.11.5 => 11.11.5 @mui/base: 5.0.0-beta.40 @mui/core-downloads-tracker: 5.15.15 @mui/icons-material: 5.15.15 => 5.15.15 @mui/material: 5.15.15 => 5.15.15 @mui/private-theming: 5.15.14 @mui/styled-engine: 5.15.14 @mui/system: 5.15.15 => 5.15.15 @mui/types: 7.2.14 @mui/utils: 5.15.14 @mui/x-data-grid: 7.3.0 @mui/x-data-grid-pro: 7.3.0 => 7.3.0 @mui/x-date-pickers: 7.1.1 => 7.1.1 @mui/x-date-pickers-pro: 7.1.1 => 7.1.1 @mui/x-license: 7.1.1 @mui/x-license-pro: 6.10.2 => 6.10.2 @types/react: 18.2.75 => 18.2.75 react: 18.2.0 => 18.2.0 react-dom: 18.2.0 => 18.2.0 typescript: 5.4.4 => 5.4.4 ```

Search keywords: grid pagination details bug Order ID: 83568

michelengelen commented 3 weeks ago

Hey @bjackson42 ... this is indeed a slight regression on our side. The responsible commit is this: 7c4ba741beeec7ba92b0d4686b6dda73594d58b6

we can easily add this back in. Here is a diff for that change to start off:

diff --git a/packages/x-data-grid/src/hooks/features/pagination/useGridPaginationModel.ts b/packages/x-data-grid/src/hooks/features/pagination/useGridPaginationModel.ts
index c482999e4..57e7deece 100644
--- a/packages/x-data-grid/src/hooks/features/pagination/useGridPaginationModel.ts
+++ b/packages/x-data-grid/src/hooks/features/pagination/useGridPaginationModel.ts
@@ -115,17 +115,20 @@ export const useGridPaginationModel = (
       }
       logger.debug("Setting 'paginationModel' to", paginationModel);

-      apiRef.current.setState((state) => ({
-        ...state,
-        pagination: {
-          ...state.pagination,
-          paginationModel: getDerivedPaginationModel(
-            state.pagination,
-            props.signature,
-            paginationModel,
-          ),
-        },
-      }));
+      apiRef.current.setState(
+        (state) => ({
+          ...state,
+          pagination: {
+            ...state.pagination,
+            paginationModel: getDerivedPaginationModel(
+              state.pagination,
+              props.signature,
+              paginationModel,
+            ),
+          },
+        }),
+        'setPaginationModel',
+      );
     },
     [apiRef, logger, props.signature],
   );

I will put this up for grabs for the community since it is a rather small change. Feel free to take this up yourself! Thanks again for raising this with us! 🙇🏼

bjackson42 commented 3 weeks ago

Thank you for the quick reply @michelengelen! I would love for the opportunity to contribute and will definitely make a PR for this. Thank you again for the fast support and details on the issue. I really appreciate the code block showing the diffs, that is awesome.

Have a wonderful and fantastic day!

bjackson42 commented 3 weeks ago

PR created -> https://github.com/mui/mui-x/pull/12923