Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[DataGrid] Add Skeleton loading overlay support #13293
Changes from 7 commits
5242c9a
a2b1bb0
d32fa8b
d45dfdd
e8c9e0f
e05cf1d
4373460
f9d5442
d745a3e
6cf0ffa
e91886f
721e78a
ceb9c7e
e98680e
09118fb
650ae4e
7c226b6
bdbfeb5
6413802
5d54f01
5e736e9
eda2557
7de9a56
027cf4b
78641bc
5f295da
0e0e8cc
5567ef3
be974f5
7bd1b49
c6f6617
6dc2f5d
65da878
c92a3a3
ba615d4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 correctsectionIndex
andsectionLength
values to theshouldCellShowRightBorder
function.Fixed here c92a3a3
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#40594There 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#L3There 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.
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.
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.