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.

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

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Jun 2, 2024

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:
image

@mui-bot
Copy link

mui-bot commented Jun 2, 2024

Deploy preview: https://deploy-preview-13349--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 63f3a7c

@sai6855 sai6855 added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Pagination Related to the data grid Pagination feature and removed bug 🐛 Something doesn't work labels Jun 2, 2024
@@ -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

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

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

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

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

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

@sai6855 sai6855 force-pushed the fix-#13330 branch 2 times, most recently from 99f6023 to 0fc580c Compare June 3, 2024 17:27
@romgrk romgrk enabled auto-merge (squash) July 4, 2024 03:05
@romgrk romgrk merged commit 5d67525 into mui:master Jul 4, 2024
15 checks passed
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
Co-authored-by: Rom Grk <romgrk.cc@gmail.com>
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Co-authored-by: Rom Grk <romgrk.cc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! feature: Pagination Related to the data grid Pagination feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] Strange behavior when paginationMode is undefined
5 participants