-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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.
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
Conversation
Deploy preview: https://deploy-preview-13349--material-ui-x.netlify.app/ |
@@ -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, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 😬
There was a problem hiding this comment.
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],
);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
99f6023
to
0fc580c
Compare
Co-authored-by: Rom Grk <[email protected]>
Co-authored-by: Rom Grk <[email protected]>
closes: #13330
preview: https://deploy-preview-13349--material-ui-x.netlify.app/x/react-data-grid/pagination/
check this #13349 (comment) dicussion for more context on implementaion of logic
after applying fix: