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] Consider bringing back the error overlay #11399

Open
Tracked by #12317
Janpot opened this issue Dec 13, 2023 · 4 comments
Open
Tracked by #12317

[DataGrid] Consider bringing back the error overlay #11399

Janpot opened this issue Dec 13, 2023 · 4 comments
Labels
component: data grid This is the name of the generic UI component, not the React module! dx Related to developers' experience waiting for 👍 Waiting for upvotes

Comments

@Janpot
Copy link
Member

Janpot commented Dec 13, 2023

Summary 💡

Mostly useful when doing serverside pagination/filtering. The main suggestion I see on the issue tracker seems to be to create an overlay for the whole grid in case a data loading error happens. The specific use-case where this setup is problematic is the one where the usage of a specific filter results in an error. One could get the grid back to the working state by simply removing the filter. but since the grid has been fully overlaid, one can't access the filter controls anymore. An error overlay that targets specifically the rows area doesn't have this problem.

Illustration of the problem: https://stackblitz.com/edit/react-xffwnt?file=Demo.tsx
in this demo, After adding a filter to this id column, the whole grid disappears behind the error overlay.

In Toolpad at the moment we've hacked around this problem by hijacking the NoRowsOverlay slot, as can be seen in the following demo: https://stackblitz.com/edit/react-xffwnt-ko8kc5?file=Demo.tsx
But this has some limitations (e.g. can't overlay the error over old data).

As a sidenote, error handling is such an important consideration in serverside loading scenarios, yet none of the DataGrid demos take it into account. The fake useQuery function simply ignores the fact that this function in react-query can return an error. In the ideal scenario, errors can be easily displayed in-context, inside of the grid itself. Since MUI X removed the error prop, we've really had to build a lot of machinery in Toolpad to handle serverside errors. Personally I don't understand why this property was removed. I now have to wrap every grid in an error boundary + a separate overlay for row loading errors. It's quite inconvenient.

Examples 🌈

No response

Motivation 🔦

No response

Search keywords: error overlay

@Janpot Janpot added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 13, 2023
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Dec 15, 2023
@michelengelen michelengelen removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 18, 2023
@MBilalShafi
Copy link
Member

MBilalShafi commented Dec 18, 2023

Thank you @Janpot for raising the issue.
If I recall it correctly we removed the error prop and ErrorOverlay when we removed the ErrorBoundary from within Data Grid for users to be able to use it outside the Grid. For reference, here's a related discussion and the problems around error handling reported which started this discussion.

IMHO, there was no other obvious reason for removing the error prop along with error boundary other than it being easier to be done on users' side and one less prop to be handled by grid. (@cherniavskii please correct me if I missed something).
So, if there's an obvious need and pain points faced by many people, we can always bring them back.

By the way, I tried to adjust your example with the known headerHeight and a little bit of tweaking in the CSS and it seems to kind-of work without hijacking the NoRowsOverlay slot. Do you think if this could possibly be a solution/direction for similar issues?

@cherniavskii
Copy link
Member

We discussed this during the weekly meeting – we'll bring back the error overlay (with a dedicated slot) and the error prop.
We won't bring the ErrorBoundary back, you'll have to wrap the DataGrid with it if necessary.
@Janpot How does it sound?

@Janpot
Copy link
Member Author

Janpot commented May 13, 2024

We won't bring the ErrorBoundary back, you'll have to wrap the DataGrid with it if necessary.

How will that work when for instance the data is missing an id field? Will it throw an error? Or will it display in the rows area? i.e. when I have a backend error while loading the rows, I'd like to show as much as possible of the grid UI (toolbar, pagination controls, column headers) and just show an error in the area where otherwise the rows would render. In my intuition, and error with the format of the row data would go in there as well. But if the grid throws in that case, this won't be possible. Or at least, I'd like to somehow be able to display missing id error there when Toolpad is in development mode (or cell formatting error,...).

@cherniavskii
Copy link
Member

How will that work when for instance the data is missing an id field? Will it throw an error? Or will it display in the rows area?

That's a good question. Yes, it will throw an error and you should be able to catch it in the error boundary:
https://codesandbox.io/p/sandbox/epic-darkness-8j4hdl

Interestingly, Data Grid v5 that had the error boundary and error overlay doesn't catch it either: https://codesandbox.io/p/sandbox/wizardly-star-8hmr8q?file=%2Fsrc%2FDemo.tsx%3A12%2C22
I believe this is because the error is thrown up in the tree before Data Grid even has a chance to render anything.

when I have a backend error while loading the rows, I'd like to show as much as possible of the grid UI (toolbar, pagination controls, column headers) and just show an error in the area where otherwise the rows would render.

In Data Grid v5, the error overlay was covering the whole Data Grid: https://stackblitz.com/edit/react-8vqwuz?file=Demo.tsx,package.json,index.tsx
I think we can make it configurable in v7 and have an error overlay that only covers the rows rendering container.
This would only work for errors passed to the Data Grid via the error prop, and won't work if the error is thrown by the Data Grid during rendering.

I'd like to somehow be able to display missing id error there when Toolpad is in development mode

I'm not sure it's possible since the missing Id error is thrown at the top of the Data Grid tree.

@cherniavskii cherniavskii added dx Related to developers' experience waiting for 👍 Waiting for upvotes labels Aug 27, 2024
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! dx Related to developers' experience waiting for 👍 Waiting for upvotes
Projects
Status: 🔖 Ready
Development

No branches or pull requests

5 participants