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.

Sign up for GitHub

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] Add unstable_setRowHeight method to apiRef #3751

Merged
merged 10 commits into from
Feb 14, 2022
Merged

[DataGridPro] Add unstable_setRowHeight method to apiRef #3751

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
58be1fe
[DataGrid] Add unstable_setRowHeight method to apiRef
cherniavskii Jan 26, 2022
038e611
add story
cherniavskii Jan 26, 2022
0297e8e
add unit tests
cherniavskii Jan 26, 2022
2832094
skip tests in jsdom
cherniavskii Jan 26, 2022
9702d25
check getRowHeight spy in test
cherniavskii Jan 28, 2022
d95381d
Merge branch 'master' into apiRef-setRowHeight
cherniavskii Feb 8, 2022
9672b27
resolve conflicts
cherniavskii Feb 8, 2022
b4260cc
code review: Matheus
cherniavskii Feb 10, 2022
ba3b81a
Merge branch 'master' into apiRef-setRowHeight
cherniavskii Feb 14, 2022
bb2c54d
fix apiRef type
cherniavskii Feb 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 33 additions & 8 deletions packages/grid/_modules_/grid/hooks/features/rows/useGridRowsMeta.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ export const useGridRowsMeta = (
props: Pick<DataGridProcessedProps, 'getRowHeight' | 'pagination' | 'paginationMode'>,
): void => {
const { getRowHeight, pagination, paginationMode } = props;
const rowsHeightLookup = React.useRef<{ [key: GridRowId]: number }>({});
const rowsHeightLookup = React.useRef<{
[key: GridRowId]: { value: number; isResized: boolean };
}>({});
const rowHeight = useGridSelector(apiRef, gridDensityRowHeightSelector);
const filterState = useGridSelector(apiRef, gridFilterStateSelector);
const paginationState = useGridSelector(apiRef, gridPaginationSelector);
Expand All @@ -50,14 +52,25 @@ export const useGridRowsMeta = (
const currentRowHeight = gridDensityRowHeightSelector(state);
const currentPageTotalHeight = rows.reduce((acc: number, row) => {
positions.push(acc);
let targetRowHeight = currentRowHeight;

if (getRowHeight) {
// Default back to base rowHeight if getRowHeight returns null or undefined.
targetRowHeight = getRowHeight({ ...row, densityFactor }) ?? currentRowHeight;
}
let targetRowHeight: number;

if (rowsHeightLookup.current[row.id] && rowsHeightLookup.current[row.id].isResized) {
// do not recalculate resized row height and use the value from the lookup
targetRowHeight = rowsHeightLookup.current[row.id].value;
} else {
targetRowHeight = currentRowHeight;

rowsHeightLookup.current[row.id] = targetRowHeight;
if (getRowHeight) {
// Default back to base rowHeight if getRowHeight returns null or undefined.
targetRowHeight = getRowHeight({ ...row, densityFactor }) ?? currentRowHeight;
}

rowsHeightLookup.current[row.id] = {
value: targetRowHeight,
isResized: false,
};
}

return acc + targetRowHeight;
}, 0);
Expand All @@ -71,7 +84,18 @@ export const useGridRowsMeta = (
}, [apiRef, pagination, paginationMode, getRowHeight]);

const getTargetRowHeight = (rowId: GridRowId): number =>
rowsHeightLookup.current[rowId] || rowHeight;
rowsHeightLookup.current[rowId]?.value || rowHeight;

const setRowHeight = React.useCallback<GridRowsMetaApi['unstable_setRowHeight']>(
(id: GridRowId, height: number) => {
rowsHeightLookup.current[id] = {
value: height,
isResized: true,
};
hydrateRowsMeta();
},
[hydrateRowsMeta],
);

// The effect is used to build the rows meta data - currentPageTotalHeight and positions.
// Because of variable row height this is needed for the virtualization
Expand All @@ -81,6 +105,7 @@ export const useGridRowsMeta = (

const rowsMetaApi: GridRowsMetaApi = {
unstable_getRowHeight: getTargetRowHeight,
unstable_setRowHeight: setRowHeight,
};

useGridApiMethod(apiRef, rowsMetaApi, 'GridRowsMetaApi');
Expand Down
7 changes: 7 additions & 0 deletions packages/grid/_modules_/grid/models/api/gridRowsMetaApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,11 @@ export interface GridRowsMetaApi {
* @ignore - do not document.
*/
unstable_getRowHeight: (id: GridRowId) => number;
/**
* Updates the height of a row.
cherniavskii marked this conversation as resolved.
Show resolved Hide resolved
* @param {GridRowId} id The id of the row.
* @param {number} height The new height.
* @ignore - do not document.
*/
unstable_setRowHeight: (id: GridRowId, height: number) => void;
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this API be public? I was left with the impression that the reason we are adding it is so that devs can target specific rows manually. If that is the case then it shouldn't have the unstable_ prefix.
If it is for internal use only I would argue that we don't need it (or at least we don't need it at this point in time).

Copy link
Member

Choose a reason for hiding this comment

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

Same feeling
This API only goal is to be used by the user, so making it private kind of kill this usecase.

Copy link
Member

Choose a reason for hiding this comment

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

It will be used when we add the the resize handle. Since we don't know if setRowHeight will work for that purpose and it won't require heavy changes I would keep private. But it should be public once we know it's stable.

Copy link
Member Author

@cherniavskii cherniavskii Jan 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Exactly what @m4theushw said.

One more thing I was thinking of is that we might change its signature to setRowsHeight to support resizing multiple rows at once. This way hydrateRowsMeta() will get called once per resize, not once per row per resize.
But maybe I should implement it like that in the first place?

unstable_setRowsHeight: (rows: Array<{ id: GridRowId, height: number }>) => void;

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case for resizing multiple rows at once? Maybe only through a button. If there's one, unstable_setRowsHeight could call the unstable_setRowHeight but have an argument to only hydrateRowsMeta after the last row. I remember we had something similar for postponing the call to applyFiters.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there a use case for resizing multiple rows at once?

I have no idea, but you can do that in Google spreadsheet for example.
I'm just trying to avoid future breaking changes (if we want to ditch unstable_ prefix now)

For updateColumns we are doing the opposite : updateColumn calls updateColumns

Yes, that's the reason I'm proposing the array signature.
Also, I think it's more elegant than setRowHeight(hydrateRowsMeta), because we don't need to explain what is hydrateRowsMeta and when to use it.

Is there an advantage of using Array over X[] ?

AFAIK those two signatures are equal, so it doesn't matter

Copy link
Member

Choose a reason for hiding this comment

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

It will be used when we add the the resize handle. Since we don't know if setRowHeight will work for that purpose and it won't require heavy changes I would keep private. But it should be public once we know it's stable.

I'm a bit confused. It seems like we are adding an API method to hypothetically cover a case that may or may not exist. I was left with the impression that this new API method will be used eventually to support row resizing (similar to column resizing) but I don't know if this is even a feature we want to add eventually.
I would be in favor of starting with a very narrow scope -> resize a target row - and make that public.

Copy link
Member

@m4theushw m4theushw Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We have two features that could leverage setRowHeight: #3801 and #417. This PR could be the first step to #417. Any spreadsheet app allows to automatically adjust the row height to the content height by double clicking the row. Instead of a full dynamic height feature, we could allow the same thing in the DataGrid. Users have been encouraged to use this demo but you can only see the full content on hover. Despite having a use for setRowHeight or not, for any new feature released we release a declarative API (props) and a imperative API (methods), for #3218 we miss the last.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not about always adding an imperative API to every feature. I think it should be on a case-by-case basis, otherwise, we will end up with too much public API, like it was before.
Either way - I'm not saying we shouldn't add that API method but rather just remove the unstable_ prefix and keep it as it is - update only 1 row.

Copy link
Member Author

@cherniavskii cherniavskii Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

On today's meeting we came to a conclusion that we are not sure about stability of this API and we'll keep unstable_ for now

}
75 changes: 74 additions & 1 deletion packages/grid/x-data-grid-pro/src/tests/rows.DataGridPro.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import * as React from 'react';
import { createRenderer, fireEvent } from '@mui/monorepo/test/utils';
import { spy } from 'sinon';
import { expect } from 'chai';
import { getCell, getRow, getColumnValues, getRows } from 'test/utils/helperFn';
import {
getCell,
getRow,
getColumnValues,
getRows,
getColumnHeaderCell,
} from 'test/utils/helperFn';
import {
GridApiRef,
GridRowModel,
Expand Down Expand Up @@ -792,4 +798,71 @@ describe('<DataGridPro /> - Rows', () => {
}).not.to.throw();
});
});

describe('apiRef: setRowHeight', () => {
const ROW_HEIGHT = 52;

before(function beforeHook() {
if (isJSDOM) {
// Need layouting
this.skip();
}
});

beforeEach(() => {
baselineProps = {
autoHeight: isJSDOM,
rows: [
{
id: 0,
brand: 'Nike',
},
{
id: 1,
brand: 'Adidas',
},
{
id: 2,
brand: 'Puma',
},
],
columns: [{ field: 'brand', headerName: 'Brand' }],
};
});

let apiRef: GridApiRef;

const TestCase = (props: Partial<DataGridProProps>) => {
apiRef = useGridApiRef();
return (
<div style={{ width: 300, height: 300 }}>
<DataGridPro {...baselineProps} apiRef={apiRef} rowHeight={ROW_HEIGHT} {...props} />
</div>
);
};

it('should change row height', () => {
render(<TestCase />);
Copy link
Member

Choose a reason for hiding this comment

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

When asserting on the row size it's a good practice to hardcode the initial row height so if the default value is changed the test doesn't break.

https://github.com/mui-org/material-ui-x/blob/07cda31650f4ddd6d39e02578b6fee9637159e5e/packages/grid/x-data-grid/src/tests/layout.DataGrid.test.tsx#L730-L736

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I've seen that in other tests

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, actually it's already done - see TestCase component


expect(getRow(1).clientHeight).to.equal(ROW_HEIGHT);

apiRef.current.unstable_setRowHeight(1, 100);
expect(getRow(1).clientHeight).to.equal(100);
});

it('should preserve changed row height after sorting', () => {
Copy link
Member

Choose a reason for hiding this comment

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

A suggestion: you could make an assertion by passing a mocked getRowHeight callback and check if it's not called after changing the height.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I've added getRowHeight check to the test.

render(<TestCase />);

const row = getRow(0);
expect(row.clientHeight).to.equal(ROW_HEIGHT);

apiRef.current.unstable_setRowHeight(0, 100);
expect(row.clientHeight).to.equal(100);

// sort
fireEvent.click(getColumnHeaderCell(0));

expect(row.clientHeight).to.equal(100);
});
});
});
66 changes: 66 additions & 0 deletions packages/storybook/src/stories/grid-rows.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import Button from '@mui/material/Button';
import Popper from '@mui/material/Popper';
import Paper from '@mui/material/Paper';
import Box from '@mui/material/Box';
import TextField from '@mui/material/TextField';
import {
GridCellValue,
GridCellParams,
Expand All @@ -18,6 +19,7 @@ import {
GridEvents,
MuiEvent,
GridEventListener,
GridSelectionModel,
} from '@mui/x-data-grid-pro';
import { useDemoData } from '@mui/x-data-grid-generator';
import { action } from '@storybook/addon-actions';
Expand Down Expand Up @@ -1072,3 +1074,67 @@ export function VariableRowHeight() {
</div>
);
}

export function SetRowHeight() {
const { data } = useDemoData({
dataSet: 'Commodity',
rowLength: 1000,
});

const [selectionModel, setSelectionModel] = React.useState<GridSelectionModel>([]);
React.useEffect(() => {
if (data.rows.length > 0) {
setSelectionModel([data.rows[0].id]);
}
}, [data.rows]);

const apiRef = useGridApiRef();

const handleSubmit = (event: React.SyntheticEvent) => {
event.preventDefault();
const target = event.target as typeof event.target & {
height: { value: string };
};

const height = Number(target.height.value);

selectionModel.forEach((id) => {
apiRef.current.unstable_setRowHeight(id, height);
});
};

return (
<div style={{ height: 600 }}>
<form style={{ display: 'flex', margin: '16px 0' }} onSubmit={handleSubmit}>
<TextField
name="height"
label="Row height"
size="small"
defaultValue="120"
sx={{ mr: 1 }}
/>
<Button type="submit" variant="outlined">
Set row height
</Button>
</form>
<DataGridPro
{...data}
apiRef={apiRef}
selectionModel={selectionModel}
onSelectionModelChange={(newModel) => setSelectionModel(newModel)}
getRowHeight={({ model }) => {
if (
model.commodity.includes('Oats') ||
model.commodity.includes('Milk') ||
model.commodity.includes('Soybean') ||
model.commodity.includes('Rice')
) {
return 80;
}

return null;
}}
/>
</div>
);
}