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

[DataGridPro] Fix column pinning layout #14966

Merged
merged 11 commits into from
Oct 23, 2024
2 changes: 1 addition & 1 deletion docs/data/data-grid/list-view/ListViewAdvanced.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,7 +258,7 @@ export default function ListViewAdvanced() {
return (
<React.Fragment>
<CSSBaseline />
<div ref={containerRef} style={{ maxWidth: '100%' }}>
<div ref={containerRef} style={{ maxWidth: '100%', height: '100%' }}>
<DataGridPremium
apiRef={apiRef}
rows={rows}
Expand Down
2 changes: 1 addition & 1 deletion docs/data/data-grid/list-view/ListViewAdvanced.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ export default function ListViewAdvanced() {
return (
<React.Fragment>
<CSSBaseline />
<div ref={containerRef} style={{ maxWidth: '100%' }}>
<div ref={containerRef} style={{ maxWidth: '100%', height: '100%' }}>
<DataGridPremium
apiRef={apiRef}
rows={rows}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ describe('<DataGridPro /> - Detail panel', () => {
await microtasks();

const virtualScrollerContent = $('.MuiDataGrid-virtualScrollerContent')!;
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
expect(virtualScrollerContent).toHaveComputedStyle({
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed height to flex-basis in /~https://github.com/mui/mui-x/pull/14966/files#diff-74adfff8c7dc341a80f5a489eb850b9d566b1be041dfdf0e47a9848609f5ac53R537, and I makes more sense to check the actual height rather than specific inline style.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment not specific to this in particular, but I feel like we often test the implementation too closely, e.g. we set a specific style and test for that style. Quite a few of our tests would be better as visual regression tests, they would be more resilient to implementation changes, and would catch actual regressions (like this one could have been caught if we had the visual regression test).

Copy link
Member Author

Choose a reason for hiding this comment

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

height: `${rowHeight + detailPanelHeight}px`,
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });

const detailPanels = $$('.MuiDataGrid-detailPanel');
expect(detailPanels[0]).toHaveComputedStyle({
Expand Down Expand Up @@ -128,11 +128,9 @@ describe('<DataGridPro /> - Detail panel', () => {
});

await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${rowHeight + 100}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${rowHeight + 100}px` });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });

const detailPanels = $$('.MuiDataGrid-detailPanel');
expect(detailPanels[0]).toHaveComputedStyle({
Expand All @@ -142,11 +140,9 @@ describe('<DataGridPro /> - Detail panel', () => {
fireEvent.click(screen.getByRole('button', { name: 'Increase' }));

await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${rowHeight + 200}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${rowHeight + 200}px` });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });

expect(detailPanels[0]).toHaveComputedStyle({
height: `200px`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -743,7 +743,7 @@ export const GridRootStyles = styled('div', {
},

[`& .${c.filler}`]: {
flex: 1,
flex: '1 0 auto',
},
[`& .${c['filler--borderBottom']}`]: {
borderBottom: '1px solid var(--DataGrid-rowBorderColor)',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ import clsx from 'clsx';
import { styled } from '@mui/system';
import composeClasses from '@mui/utils/composeClasses';
import { gridClasses, getDataGridUtilityClass } from '../../constants/gridClasses';
import { gridDimensionsSelector } from '../../hooks/features/dimensions/gridDimensionsSelectors';
import { useGridApiContext } from '../../hooks/utils/useGridApiContext';
import { useGridSelector } from '../../hooks/utils/useGridSelector';

const useUtilityClasses = () => {
const slots = {
Expand All @@ -23,25 +20,10 @@ const Element = styled('div')({
export function GridBottomContainer(props: React.PropsWithChildren) {
const classes = useUtilityClasses();

const apiRef = useGridApiContext();
const { viewportOuterSize, minimumSize, hasScrollX, scrollbarSize } = useGridSelector(
apiRef,
gridDimensionsSelector,
);
const scrollHeight = hasScrollX ? scrollbarSize : 0;
const offset = Math.max(
viewportOuterSize.height -
minimumSize.height -
// Subtract scroll height twice to account for GridVirtualScrollerFiller and horizontal scrollbar
2 * scrollHeight,
0,
);

return (
<Element
{...props}
className={clsx(classes.root, gridClasses['container--bottom'])}
style={{ transform: `translateY(${offset}px)` }}
Copy link
Member Author

Choose a reason for hiding this comment

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

No need for that because filler now grows

role="presentation"
/>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ const Scroller = styled('div', {
flexGrow: 1,
overflow: 'scroll',
scrollbarWidth: 'none' /* Firefox */,
display: 'flex',
flexDirection: 'column',
'&::-webkit-scrollbar': {
display: 'none' /* Safari and Chrome */,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,8 @@ export const useGridVirtualScroller = () => {
const contentSize = React.useMemo(() => {
const size: React.CSSProperties = {
width: needsHorizontalScrollbar ? columnsTotalWidth : 'auto',
height: contentHeight,
flexBasis: contentHeight,
flexShrink: 0,
};

if (rootProps.autoHeight && currentPage.rows.length === 0) {
Expand Down
50 changes: 18 additions & 32 deletions packages/x-data-grid/src/tests/rows.DataGrid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -645,11 +645,9 @@ describe('<DataGrid /> - Rows', () => {
);
const expectedHeight = baselineProps.rows.length * (contentHeight + border);
await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${expectedHeight}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${expectedHeight}px` });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should use the default row height to calculate the content size when the row has not been measured yet', async () => {
Expand All @@ -674,11 +672,9 @@ describe('<DataGrid /> - Rows', () => {
border + // Measured rows also include the border
(baselineProps.rows.length - 1) * defaultRowHeight;
await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${expectedHeight}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${expectedHeight}px` });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should use the value from getEstimatedRowHeight to estimate the content size', async () => {
Expand All @@ -703,11 +699,9 @@ describe('<DataGrid /> - Rows', () => {
const expectedHeight =
firstRowHeight + (baselineProps.rows.length - 1) * estimatedRowHeight;
await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${expectedHeight}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${expectedHeight}px` });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should recalculate the content size when the rows prop changes', async () => {
Expand All @@ -723,18 +717,14 @@ describe('<DataGrid /> - Rows', () => {
'.MuiDataGrid-virtualScrollerContent',
);
await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: '101px',
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: '101px' });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
setProps({ rows: [{ clientId: 'c1', expanded: true }] });
await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: '201px',
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: '201px' });
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should set minHeight to "auto" in all rows with dynamic row height', () => {
Expand Down Expand Up @@ -813,11 +803,11 @@ describe('<DataGrid /> - Rows', () => {
'.MuiDataGrid-virtualScrollerContent',
)!;
await waitFor(() => {
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
expect(virtualScrollerContent).toHaveComputedStyle({
height: `${Math.floor(expectedHeight)}px`,
});
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should position correctly the render zone when the 2nd page has less rows than the 1st page', async function test() {
Expand Down Expand Up @@ -981,10 +971,8 @@ describe('<DataGrid /> - Rows', () => {
);
const virtualScrollerContent = document.querySelector('.MuiDataGrid-virtualScrollerContent');
const expectedHeight = rows.length * (rowHeight + spacingTop + spacingBottom);
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${expectedHeight}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${expectedHeight}px` });
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should update the content size when getRowSpacing is removed', () => {
Expand All @@ -1000,15 +988,13 @@ describe('<DataGrid /> - Rows', () => {
);
const virtualScrollerContent = document.querySelector('.MuiDataGrid-virtualScrollerContent');
const expectedHeight = rows.length * (rowHeight + spacingTop + spacingBottom);
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
height: `${expectedHeight}px`,
});
expect(virtualScrollerContent).toHaveComputedStyle({ height: `${expectedHeight}px` });
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
setProps({ getRowSpacing: null });
expect(virtualScrollerContent).toHaveInlineStyle({
width: 'auto',
expect(virtualScrollerContent).toHaveComputedStyle({
height: `${rows.length * rowHeight}px`,
});
expect(virtualScrollerContent).toHaveInlineStyle({ width: 'auto' });
});

it('should set the row margin to the value returned by getRowSpacing if rowSpacingType is not defined', () => {
Expand Down
62 changes: 62 additions & 0 deletions test/regressions/data-grid/DataGridPinnedColumns.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import * as React from 'react';
import DeleteIcon from '@mui/icons-material/Delete';
import EditIcon from '@mui/icons-material/Edit';
import { DataGridPro, GridActionsCellItem } from '@mui/x-data-grid-pro';
import {
randomCreatedDate,
randomTraderName,
randomEmail,
randomUpdatedDate,
} from '@mui/x-data-grid-generator';

const columns = [
{ field: 'name', headerName: 'Name', width: 160, editable: true },
{ field: 'email', headerName: 'Email', width: 200, editable: true },
{ field: 'age', headerName: 'Age', type: 'number', editable: true },
{
field: 'dateCreated',
headerName: 'Date Created',
type: 'date',
width: 180,
editable: true,
},
{
field: 'lastLogin',
headerName: 'Last Login',
type: 'dateTime',
width: 220,
editable: true,
},
{
field: 'actions',
type: 'actions',
width: 100,
getActions: () => [
<GridActionsCellItem icon={<EditIcon />} label="Edit" />,
<GridActionsCellItem icon={<DeleteIcon />} label="Delete" />,
],
},
];

const rows = [
{
id: 1,
name: randomTraderName(),
email: randomEmail(),
age: 25,
dateCreated: randomCreatedDate(),
lastLogin: randomUpdatedDate(),
},
];

export default function BasicColumnPinning() {
return (
<div style={{ height: 400, width: '100%' }}>
<DataGridPro
rows={rows}
columns={columns}
initialState={{ pinnedColumns: { left: ['name'], right: ['actions'] } }}
/>
</div>
);
}