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.

Sign up for GitHub

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

[DataGrid] Fix pagination when pagination={undefined} #13349

Merged
merged 5 commits into from
Jul 4, 2024
Merged

[DataGrid] Fix pagination when pagination={undefined} #13349

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2aa6306
Fix pagination when `pagination={undefined}`
sai6855 Jun 2, 2024
b7e28c2
add test
sai6855 Jun 2, 2024
a7f4bea
refactor
sai6855 Jun 3, 2024
caf78ab
fix test
sai6855 Jun 3, 2024
63f3a7c
ci: run
romgrk Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/x-data-grid/src/DataGrid/useDataGridProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export const useDataGridProps = <R extends GridValidRowModel>(inProps: DataGridP
() => ({
...DATA_GRID_PROPS_DEFAULT_VALUES,
...themedProps,
paginationMode: themedProps.paginationMode ?? DATA_GRID_PROPS_DEFAULT_VALUES.paginationMode,
Copy link
Contributor Author

@sai6855 sai6855 Jun 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

when pagination={undefined} is set, pagintionMode key will be present in themedProps and overrides paginationMode from DATA_GRID_PROPS_DEFAULT_VALUES but when pagination prop is not explicitly passed, then value of paginationMode from DATA_GRID_PROPS_DEFAULT_VALUES will be considered since themedProps won't have paginationMode key.

With below change, both not passing pagintionMode and setting pagintionMode={undefined} will behave same

paginationMode: themedProps.paginationMode ?? DATA_GRID_PROPS_DEFAULT_VALUES.paginationMode

Copy link
Member

Choose a reason for hiding this comment

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

This won't solve the problem for other props so I'm not sure it's the right way of doing it 😬

Copy link
Contributor Author

@sai6855 sai6855 Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I see, what do you think of below approach

  const injectDefaultProps = React.useMemo(() => {
    return Object.keys(DATA_GRID_PROPS_DEFAULT_VALUES).reduce((acc, key) => {
      acc[key] =
        themedProps[key as keyof DataGridProps<R>] ??
        DATA_GRID_PROPS_DEFAULT_VALUES[key as keyof DataGridPropsWithDefaultValues<any>];
      return acc;
    }, {} as DataGridPropsWithDefaultValues<any>);
  }, [themedProps]);

  return React.useMemo<DataGridProcessedProps<R>>(
    () => ({
-      ...DATA_GRID_PROPS_DEFAULT_VALUES,
      ...themedProps,
+     ...injectDefaultProps,
      localeText,
      slots,
      ...DATA_GRID_FORCED_PROPS,
    }),
    [themedProps, localeText, slots, injectDefaultProps],
  );

Copy link
Member

@flaviendelangle flaviendelangle Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah we probably need something like that
I'll let the team working on the grid review the exact implementation.

By the way I wouldn't be surprised if we had similar issues on other packages such as @mui/x-date-pickers, I'll have a look once we settle on something here.

Copy link
Member

@Janpot Janpot Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Reminds me a bit of lodash defaults as well. A utility we could take inspiration from?

Copy link
Contributor Author

@sai6855 sai6855 Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

refactored logic from just fixing paginationMode={undefined} to fixing all defaultProps. commit: a7f4bea

localeText,
slots,
...DATA_GRID_FORCED_PROPS,
Expand Down
6 changes: 6 additions & 0 deletions packages/x-data-grid/src/tests/pagination.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,4 +751,10 @@ describe('<DataGrid /> - Pagination', () => {
].join('\n'),
);
});

it('should not log an error if paginationMode is set to undefined', () => {
expect(() => {
render(<BaselineTestCase paginationMode={undefined} />);
}).not.toErrorDev();
});
});