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

[DataGridPremium] Fix loading issue + add skeleton overlay #16282

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

MBilalShafi
Copy link
Member

@MBilalShafi MBilalShafi commented Jan 21, 2025

@MBilalShafi MBilalShafi added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Aggregation Related to the data grid Aggregation feature feature: Server integration Better integration with backends, e.g. data source labels Jan 21, 2025
@mui-bot
Copy link

mui-bot commented Jan 21, 2025

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

Generated by 🚫 dangerJS against dc3a32c

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 29, 2025
@MBilalShafi MBilalShafi changed the title [DataGridPremium] Fix initial loading state not being displayed [DataGridPremium] Fix loading issue + add skeleton overlay Jan 29, 2025
Copy link
Contributor

@arminmeh arminmeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great 💯


const visibleColumns = new Set(Object.keys(rootLookup));
const viewportHeight = dimensions.bottomContainerHeight ?? 0;
const skeletonRowsCount = Math.ceil(viewportHeight / dimensions.rowHeight);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not always 1?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, I wanted to use this for all the pinned rows in the container, but turned out the pinned rows don't support lazy loading by default, so I made it specific to the aggregation row. We could certainly try limiting it to 1.

Copy link
Member

@KenanYusuf KenanYusuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spotted a couple of minor things but it looks great 👍

};

const GridAggregationRowOverlay = forwardRef<HTMLDivElement, React.HTMLAttributes<HTMLDivElement>>(
function GridSkeletonLoadingOverlay(props, forwardedRef) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function GridSkeletonLoadingOverlay(props, forwardedRef) {
function GridAggregationRowOverlay(props, forwardedRef) {

const GridAggregationRowOverlayWrapper = styled('div', {
name: 'MuiDataGrid',
slot: 'AggregationRowOverlayWrapper',
overridesResolver: (_, styles) => styles.aggregationRowOverlayWrapper,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
overridesResolver: (_, styles) => styles.aggregationRowOverlayWrapper,

return (
<div className={clsx(classes.root, gridClasses[`pinnedRows--${position}`])} role="presentation">
{pinnedRows}
{position === 'bottom' && <GridAggregationRowOverlay />}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a missing border with pinned row cells when also in a pinned column:
Screenshot 2025-01-30 at 10 36 21

Should look like:
Screenshot 2025-01-30 at 10 41 22


export const DATA_GRID_PREMIUM_DEFAULT_SLOTS_COMPONENTS: GridPremiumSlotsComponent = {
...DATA_GRID_PRO_DEFAULT_SLOTS_COMPONENTS,
...materialSlots,
columnMenu: GridPremiumColumnMenu,
pinnedRows: GridPinnedRows,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of interest, what's the benefit to this being a slot?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just not to spill the logic related to the Premium package in Pro or Community.

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: Aggregation Related to the data grid Aggregation feature feature: Server integration Better integration with backends, e.g. data source
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data grid] loading does not work with dataSource in DataGridPremium
4 participants