[DataGridPro] Add `unstable_setRowHeight` method to `apiRef` #3751

cherniavskii · 2022-01-26T18:06:30Z

Preview: https://deploy-preview-3751--material-ui-x.netlify.app/storybook/?path=/story/datagridpro-test-rows--set-row-height

Closes #3750

I've prefixed it with unstable - same as unstable_getRowHeight. I assume it's considered a private API for now.

mui-bot · 2022-01-26T18:17:28Z

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	213.2	411	295.2	292.26	68.681
Sort 100k rows	ms	413.9	801.1	712	631.56	150.645
Select 100k rows	ms	147.5	293.6	190.1	203.52	48.365
Deselect 100k rows	ms	107.3	260.2	178.7	179.28	51.814

Generated by 🚫 dangerJS against bb2c54d

m4theushw · 2022-01-28T00:48:39Z

packages/grid/x-data-grid-pro/src/tests/rows.DataGridPro.test.tsx

+      expect(getRow(1).clientHeight).to.equal(100);
+    });
+
+    it('should preserve changed row height after sorting', () => {


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

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

m4theushw · 2022-01-28T00:52:14Z

packages/grid/x-data-grid-pro/src/tests/rows.DataGridPro.test.tsx

+    };
+
+    it('should change row height', () => {
+      render(<TestCase />);


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

Right, I've seen that in other tests

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

DanailH · 2022-01-28T10:44:41Z

packages/grid/_modules_/grid/models/api/gridRowsMetaApi.ts

+   * @param {number} height The new height.
+   * @ignore - do not document.
+   */
+  unstable_setRowHeight: (id: GridRowId, height: number) => void;


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).

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

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.

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?

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.

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

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.

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.

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.

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

github-actions · 2022-02-03T15:23:56Z

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

siriwatknp · 2022-02-08T12:24:22Z

👋 The migration PR has been merged.

Please follow these steps to make sure that the contents are all updated. (Sorry for the inconvenience)

pull latest master from upstream to your branch
if your PR has changes on the *.md or demo files, you are likely to encounter conflict because all of them have been moved to the new folder.
2.1 revert the change on those markdown files you did
2.2 pull latest master from upstream (you should not see conflict)
2.3 make the changes again at docs/data/data-grid/*
run yarn docs:api
- you might see the changes in docs/pages/x/api/data-grid/* if your PR touches some of the components
- it is okay if there is no changes

If you are struggle with the steps above, feel free to tag @siriwatknp

packages/grid/_modules_/grid/models/api/gridRowsMetaApi.ts

packages/grid/x-data-grid-pro/src/tests/rows.DataGridPro.test.tsx

github-actions · 2022-02-14T09:22:34Z

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

cherniavskii added 3 commits January 26, 2022 18:19

[DataGrid] Add unstable_setRowHeight method to apiRef

58be1fe

add story

038e611

add unit tests

0297e8e

cherniavskii requested review from DanailH and removed request for DanailH January 26, 2022 18:07

skip tests in jsdom

2832094

cherniavskii requested review from DanailH and flaviendelangle January 26, 2022 19:01

cherniavskii added the component: data grid This is the name of the generic UI component, not the React module! label Jan 27, 2022

m4theushw reviewed Jan 28, 2022

View reviewed changes

check getRowHeight spy in test

9702d25

DanailH reviewed Jan 28, 2022

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 3, 2022

cherniavskii added 2 commits February 8, 2022 20:56

Merge branch 'master' into apiRef-setRowHeight

d95381d

resolve conflicts

9672b27

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 8, 2022

cherniavskii requested a review from m4theushw February 9, 2022 08:42

m4theushw approved these changes Feb 9, 2022

View reviewed changes

packages/grid/_modules_/grid/models/api/gridRowsMetaApi.ts Outdated Show resolved Hide resolved

packages/grid/x-data-grid-pro/src/tests/rows.DataGridPro.test.tsx Outdated Show resolved Hide resolved

code review: Matheus

b4260cc

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 14, 2022

Merge branch 'master' into apiRef-setRowHeight

ba3b81a

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 14, 2022

fix apiRef type

bb2c54d

cherniavskii changed the title ~~[DataGridPro] Add unstable_setRowHeight method to apiRef~~ [DataGridPro] Add unstable_setRowHeight method to apiRef Feb 14, 2022

cherniavskii added the feature: Rendering layout Related to the data grid Rendering engine label Feb 14, 2022

cherniavskii merged commit 661b229 into mui:master Feb 14, 2022

cherniavskii deleted the apiRef-setRowHeight branch February 14, 2022 13:17

romgrk mentioned this pull request Oct 12, 2024

[DataGrid] Fix scroll jumping #14929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGridPro] Add `unstable_setRowHeight` method to `apiRef` #3751

[DataGridPro] Add `unstable_setRowHeight` method to `apiRef` #3751

cherniavskii commented Jan 26, 2022 •

edited by m4theushw

Loading

mui-bot commented Jan 26, 2022 •

edited

Loading

m4theushw Jan 28, 2022

cherniavskii Jan 28, 2022

m4theushw Jan 28, 2022

cherniavskii Jan 28, 2022

cherniavskii Jan 28, 2022

DanailH Jan 28, 2022

flaviendelangle Jan 28, 2022

m4theushw Jan 28, 2022

cherniavskii Jan 28, 2022 •

edited

Loading

m4theushw Feb 1, 2022

cherniavskii Feb 1, 2022

DanailH Feb 2, 2022

m4theushw Feb 2, 2022 •

edited

Loading

DanailH Feb 3, 2022

cherniavskii Feb 7, 2022 •

edited

Loading

github-actions bot commented Feb 3, 2022

siriwatknp commented Feb 8, 2022 •

edited

Loading

github-actions bot commented Feb 14, 2022

[DataGridPro] Add unstable_setRowHeight method to apiRef #3751

[DataGridPro] Add unstable_setRowHeight method to apiRef #3751

Conversation

cherniavskii commented Jan 26, 2022 • edited by m4theushw Loading

mui-bot commented Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cherniavskii Jan 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

m4theushw Feb 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cherniavskii Feb 7, 2022 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2022

siriwatknp commented Feb 8, 2022 • edited Loading

github-actions bot commented Feb 14, 2022

[DataGridPro] Add `unstable_setRowHeight` method to `apiRef` #3751

[DataGridPro] Add `unstable_setRowHeight` method to `apiRef` #3751

cherniavskii commented Jan 26, 2022 •

edited by m4theushw

Loading

mui-bot commented Jan 26, 2022 •

edited

Loading

cherniavskii Jan 28, 2022 •

edited

Loading

m4theushw Feb 2, 2022 •

edited

Loading

cherniavskii Feb 7, 2022 •

edited

Loading

siriwatknp commented Feb 8, 2022 •

edited

Loading