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

[DataGrid] Variable row height #3218

Merged
merged 57 commits into from
Jan 25, 2022

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Nov 18, 2021

Fixes #438

Preview -> https://deploy-preview-3218--material-ui-x.netlify.app/components/data-grid/rows/#variable-row-height

CHANGELOG entry for the release

🚣 Introduce variable row height (#438) @DanailH

Allow for setting a row-specific height. Before all rows had the same height, now you can set the height on a per-row basis.

<DataGrid
  getRowHeight={({ id }: GridRowHeightParams) => id % 2 === 0 ? 100 : null}
/>

ToDo:

  • Tests
  • Docs

@DanailH DanailH added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Nov 18, 2021
@DanailH DanailH self-assigned this Nov 18, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 24, 2021
@github-actions
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 Nov 29, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 3, 2021
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

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 Dec 6, 2021
@github-actions
Copy link

github-actions bot commented Dec 9, 2021

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

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Dec 9, 2021
@DanailH
Copy link
Member Author

DanailH commented Dec 9, 2021

Ok ... I would really like a review of what has been done so far. The functionality is almost ready but I'm struggling with fixing the failing tests. I'm probably missing a key interaction because this is closely tight to the virtualization and is impacted by every functionality that changes the rows in a way (sorting, filtering, pagination, treeData, density, etc). If someone can spare a pair of eyes to check it I would really appreciate it.

I skipped 2 tests (both are the same, related to the translation using the Theme) - for some reason it enters an infinite loop there.

Another thing that I don't really get is how are the tests from yarn test:karma passing and the ones from yarn test:coverage failing?

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

I didn't look into the functionality yet. However, I'm already using some parts in #3387.

I don't think we should treat variable row height as a feature, the column variable width is not too.

For the remaining tests that are failing only with yarn test:unit, you can fix them by using disableVirtualization. They fail because in jsdom rootRef.current!.clientHeight! is 0. You can refer to the master/detail PR because the CI is green there and I'm reusing most part of your work here.

@DanailH DanailH force-pushed the feature/DataGrid-variable-row-height branch from 6aef23a to ca386fa Compare December 10, 2021 13:56
if (!dimensions) {
return 0;
}
const renderContext = apiRef.current.unstable_getRenderContext();
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 a huge fan of exposing virtualization internals upper in the DataGrid component tree.
But I don't have a good solution either.

If @m4theushw has a better idea.
Otherwise, I'm fine with merging as is and taking time later to improve.

Copy link
Member

@m4theushw m4theushw Jan 21, 2022

Choose a reason for hiding this comment

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

Actually the keyboard navigation won't work as expected if disableVirtualization=true. With virtualization disabled, the last row index in the render context points to the last row in the entire row set. I'm waiting for #3667 to be merged to able to test if there're no regressions. In the worst scenario, I would keep the old behavior if getRowHeight is not being used.

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 tried calculating it without the renderContext -> the problem is that because of the variable row height you need to know the indexes of the first and last rows in the viewport to calculate the total number of visible rows. If anyone have any ideas I'm open to them.

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 regression here. Pressing Page Up is jumping directly to the column header while not on the first row yet. I don't mind if we ship this small bug only affecting those users using getRowHeight, but keeping the old behavior for those who don't use it. I think the correct way would be to base on rowsMeta.positions and the current scroll position.

msedge_srHruiStdO

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated!

@github-actions
Copy link

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

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

I've resolved a conflict caused by merging #3667

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 24, 2022
@github-actions
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 24, 2022
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

Well done.

@DanailH DanailH merged commit e7ac270 into mui:master Jan 25, 2022
@DanailH
Copy link
Member Author

DanailH commented Jan 25, 2022

Merged. I'll update the PR description for the release.

@m4theushw
Copy link
Member

Although we merged, there's still room for improvement. There're 3 things we could do in the future:

  • Add an apiRef.current.setRowHeight method to the API to imperatively change the row height, similar to the one available to control the column width

  • Fix keyboard navigation, e.g. Page Up, to use the scroll position and the position of each row

  • Add a resize handle to the rows to allow to change their height. It's the same thing we have for the columns. We need apiRef.current.setRowHeight for that.

@flaviendelangle
Copy link
Member

I created an issue to keep track of those subjects
#3737

@oliviertassinari
Copy link
Member

A thought on the getRowHeight API signature: mui/material-ui#32766 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] Variable row height
6 participants