-
-
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] Add Skeleton loading overlay support #13293
Conversation
Deploy preview: https://deploy-preview-13293--material-ui-x.netlify.app/ Updated pages: |
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
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.
Nice, a quick review on the code
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
import * as React from 'react'; | ||
import Skeleton from '@mui/material/Skeleton'; | ||
|
||
import { styled } from '@mui/system'; |
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.
We should import from Material UI here no? Same for all instances, until we deprecate @mui/material/styles
for @mui/system
. But until then, we need the theme. mui/material-ui#40594
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.
@oliviertassinari I wasn't sure because I found other instances in the same directory where styled
is imported from @mui/system
e.g. https://github.com/mui/mui-x/blob/master/packages/x-data-grid/src/components/base/GridOverlays.tsx#L3
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.
@oliviertassinari I had a similar discussion on Slack last week with quite a different answer: https://mui-org.slack.com/archives/C0170JAM7ML/p1717077013655759
It would be great to align on this topic to avoid back and forth between those two import paths
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 sounds compatible, if the style of the component needs to access the theme directly, it must import from @mui/material/styles
for now, since we don't force people to provide a theme, it needs the default value for the theme value accessed. Otherwise, import from @mui/system
.
In the future, everything can be imported from @mui/system
(meaning Pigment CSS).
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.
In the future, everything can be imported from @mui/system (meaning Pigment CSS).
Is there a timeline for this? With v6?
It might impact a priority OKR for the Data Grid where we want to remove all @mui/material
imports.
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.
Is there a timeline for this? With v6?
I don't know. I'm actually not even sure it's the direction taken by the MUI System team. cc @brijeshb42 and @siriwatknp. At least, in my mind. @mui/material should include nothing else than Base UI components that are styled. I could see Pigment System as where all the theming and specific styling things of Material UI to be hosted or have it under a separate brand like Material UI System.
Maybe there is a simple solution to the scroll sync issue, we solved this with a sticky position for the main grid, but maybe it doesn't matter much, we could sleep on this: Screen_Recording_20240530_223202_Chrome_1.mp4 |
If we want it to scroll in sync, the skeleton needs to be a child of the scroll container. Javascript can never scroll stuff in sync, because the |
….tsx Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
….tsx Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
import * as React from 'react'; | ||
import Skeleton from '@mui/material/Skeleton'; | ||
|
||
import { styled } from '@mui/system'; |
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.
@oliviertassinari I had a similar discussion on Slack last week with quite a different answer: https://mui-org.slack.com/archives/C0170JAM7ML/p1717077013655759
It would be great to align on this topic to avoid back and forth between those two import paths
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
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.
Looks great!
minWidth: '100%', | ||
width: 'max-content', // prevents overflow: clip; cutting off the x axis | ||
height: '100%', | ||
overflow: 'clip', // y axis is hidden while the x axis is allowed to overflow |
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.
Oh nice, you managed to fix the gap with the right-pinned column 👍🏻
Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com> Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com>
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.
Great work @KenanYusuf 🎉
Overall, it feels pretty solid. I added a few minor comments.
packages/x-data-grid/src/components/GridSkeletonLoadingOverlay.tsx
Outdated
Show resolved
Hide resolved
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.
Could you confirm that it works well with the showCellVerticalBorder
prop?
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.
Tested and found that it worked well with the showCellVerticalBorder
prop when it was not overflowing horizontally, but the right border disappeared when it became scrollable. This turned out to be because I was not providing the correct sectionIndex
and sectionLength
values to the shouldCellShowRightBorder
function.
Fixed here c92a3a3
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
If you want to customize the no rows overlay, a component can be passed to the `loadingOverlay` slot. In the following demo, a [LinearProgress](/material-ui/react-progress/#linear) component is rendered in place of the default circular loading spinner. | ||
If you want to customize the no rows overlay, a component can be passed to the `loadingOverlay` slot. | ||
|
||
In the following demo, a labelled determinate [CircularProgress](/material-ui/react-progress/#circular-determinate) component is rendered in place of the default loading overlay, with some additional _Loading rows…_ text. |
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.
labelled -> labeled
en-GB -> en-US
Fixed in #14025 😁
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.
Very clean. I also love that the layout is stable to rerenders https://codesandbox.io/s/epic-chatelet-43pmlk?file=/src/Demo.tsx but changes when remounted.
* Uses the grid state to determine which overlay to display. | ||
* Returns the active overlay type and the active loading overlay variant. | ||
*/ | ||
export const useGridOverlays = () => { |
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.
Default to named function in top level scope
export const useGridOverlays = () => { | |
function useGridOverlays( |
<div | ||
data-field={field} | ||
className={clsx(classes.root, className)} | ||
style={{ height, maxWidth: width, minWidth: width, ...style }} |
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.
Off-topic
I never quite understood why Damien first wrote the cells with minWidth
+ maxWidth
instead of only width
. I suspect a simplification opportunity.
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.
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.
Oh, interesting, I have tried to reproduce the issue and couldn't, so #14448 😁, I don't know, why not;
import * as React from 'react'; | ||
import Skeleton from '@mui/material/Skeleton'; | ||
|
||
import { styled } from '@mui/system'; |
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.
Is there a timeline for this? With v6?
I don't know. I'm actually not even sure it's the direction taken by the MUI System team. cc @brijeshb42 and @siriwatknp. At least, in my mind. @mui/material should include nothing else than Base UI components that are styled. I could see Pigment System as where all the theming and specific styling things of Material UI to be hosted or have it under a separate brand like Material UI System.
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Kenan Yusuf <kenan.m.yusuf@gmail.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Changes
Closes #1685
GridLoadingOverlay
to have two variant props:variant
- the loading overlay type that displays when theloading
prop is true and there are already rows in the data gridnoRowsVariant
- the loading overlay type that displays when theloading
prop is true and there are no rows in the data gridcircular-progress
,linear-progress
andskeleton
circular-progress
by default to avoid this changing the default behaviour for users unexpectedlyGridSkeletonLoadingOverlay
component to render a dynamic loading skeletonGridSkeletonCell
to consider data type when generating a random width to vary the size slightlyUsage
The variants can be set via slotProps:
Loading variants
skeleton.mov
circular-progress.mov
linear-progress.mov
Preview: https://deploy-preview-13293--material-ui-x.netlify.app/x/react-data-grid/overlays/