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

[data grid] loading does not work with dataSource in DataGridPremium #16220

Open
michael-land opened this issue Jan 16, 2025 · 10 comments · May be fixed by #16282
Open

[data grid] loading does not work with dataSource in DataGridPremium #16220

michael-land opened this issue Jan 16, 2025 · 10 comments · May be fixed by #16282
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Server integration Better integration with backends, e.g. data source

Comments

@michael-land
Copy link

michael-land commented Jan 16, 2025

Steps to reproduce

Steps:

  1. Open this link to live example: https://codesandbox.io/p/sandbox/intelligent-leavitt-q4h68n?from-embed=&workspaceId=ws_96AUJAP4MpFUq5gdSxRein
  2. Click reload
CleanShot.2025-01-16.at.17.24.55.mp4

Current behavior

"no rows" text displayed

Expected behavior

show loading spinner or skeleton

Context

No response

Your environment

npx @mui/envinfo
  System:
    OS: macOS 15.3
  Binaries:
    Node: 20.11.0 - ~/Library/Caches/fnm_multishells/97945_1737063734686/bin/node
    npm: 10.2.4 - ~/Library/Caches/fnm_multishells/97945_1737063734686/bin/npm
    pnpm: 9.15.4 - ~/Library/Caches/fnm_multishells/97945_1737063734686/bin/pnpm
  Browsers:
    Chrome: 131.0.6778.265
    Edge: Not Found
    Safari: 18.3
  npmPackages:
    @emotion/react: 11.14.0 => 11.14.0 
    @emotion/styled: 11.14.0 => 11.14.0 
    @mui/icons-material: 6.0.2 => 6.0.2 
    @mui/material: 6.4.0 => 6.4.0 
    @mui/utils: 6.4.0 => 6.4.0 
    @mui/x-data-grid-generator: 7.23.6 => 7.23.6 
    @mui/x-data-grid-premium: 8.0.0-alpha.8 => 8.0.0-alpha.8 
    @mui/x-date-pickers-pro: 8.0.0-alpha.8 => 8.0.0-alpha.8 
    @types/react: 19.0.7 => 19.0.7 
    react: 19.0.0 => 19.0.0 
    react-dom: 19.0.0 => 19.0.0 ```
</details>



**Search keywords**: dataSource
@michael-land michael-land added bug 🐛 Something doesn't work status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 16, 2025
@michael-land michael-land changed the title dataSource DataGrid#loading does not work with DataGridPremium DataGrid#loading does not work with dataSource in DataGridPremium Jan 16, 2025
@michelengelen
Copy link
Member

Could you please make that codesandbox public?

@michelengelen michelengelen added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2025
@michelengelen michelengelen changed the title DataGrid#loading does not work with dataSource in DataGridPremium [data grid] loading does not work with dataSource in DataGridPremium Jan 17, 2025
@michelengelen michelengelen added status: waiting for author Issue with insufficient information feature: Server integration Better integration with backends, e.g. data source and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 17, 2025
@michael-land
Copy link
Author

Could you please make that codesandbox public?

opps, public now

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Jan 17, 2025
@michelengelen
Copy link
Member

All right ... thanks for opening it up.
It looks like the premium grid does not use the skeleton overlay by default.
If used with this slotProps entry ...

slotProps={{
  loadingOverlay: {
    variant: 'linear-progress',
    noRowsVariant: 'skeleton',
  },
}}

... it does.

@KenanYusuf what would you expect the default to be? We should definitely align the usage across the different versions!

@michelengelen michelengelen removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jan 20, 2025
@github-project-automation github-project-automation bot moved this to 🆕 Needs refinement in MUI X Data Grid Jan 20, 2025
@KenanYusuf
Copy link
Member

Had a quick look, it's getting the correct loading variant, but the internal loading state on <DataGridPro /> is false which is why we see the "No rows" overlay. @MBilalShafi @arminmeh does anything spring to mind in the data source code?

@arminmeh
Copy link
Contributor

It looks like that some other hook removes the data source's loading overlay.
Moving the hook in the premium package to be at the same position as in the pro package solves the issue.

But before any change, I would wait for the input from @MBilalShafi, since he recently moved this hook up (I assume to be before useGridAggregation)

@MBilalShafi
Copy link
Member

MBilalShafi commented Jan 21, 2025

I assume to be before useGridAggregation

Yes, initializing an aggregation model tries to compute the initial aggregation state which uses a data source API method (resolveGroupAggregation()), I think we could skip computing the aggregation values initially since the data from the API would not be available yet and it makes more sense to keep this computation event based.

The loading state has been reset by the useGridRows and moving the data source hook after it seems to resolve the issue, I've reverted the hooks' original order and set an empty value for the initialized aggregation model: #16282

@KenanYusuf @arminmeh Does - as an initial value looks good to you or would you recommend something like Loading... or an icon that depicts the value still being loaded?

Image

@MBilalShafi MBilalShafi moved this from 🆕 Needs refinement to 👀 In review in MUI X Data Grid Jan 21, 2025
@arminmeh
Copy link
Contributor

arminmeh commented Jan 21, 2025

Does - as an initial value looks good to you or would you recommend something like Loading... or an icon that depicts the value still being loaded?

Can we also have skeleton row pill in there?

@MBilalShafi
Copy link
Member

MBilalShafi commented Jan 21, 2025

Can we also have skeleton row pill in there?

Hmm, that could be a decent option indeed. Do you think it'll make sense to add on all the cells or only for the cells with aggregation values being loaded (like Commodities and Quantity in above SS)?

For all the cells, it would be more aligned with the design language of the page but it would also show pills for the cells that have no aggregation values.
For only the cells with aggregation values, it would correctly represent the cells that are loading but would look a bit less aligned with the design language of the page.

@arminmeh
Copy link
Contributor

For me both are fine. If I have to choose, I would go with the pills only on the cells that will show the value

@KenanYusuf
Copy link
Member

KenanYusuf commented Jan 21, 2025

Can we also have skeleton row pill in there?

This would be ideal!

Do you think it'll make sense to add on all the cells or only for the cells with aggregation values being loaded

Generally, I don't think loading skeletons need to be super accurate representations of the data that is loading, but since we know which columns have aggregated values and we are already showing a partial loaded state with the column title and aggregation label, I would go for only showing it in cells with aggregation values.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Server integration Better integration with backends, e.g. data source
Projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

5 participants