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] Fix autoPageSize #1366

Merged
merged 6 commits into from
Apr 13, 2021
Merged

[DataGrid] Fix autoPageSize #1366

merged 6 commits into from
Apr 13, 2021

Conversation

dtassone
Copy link
Member

@dtassone dtassone commented Apr 6, 2021

When autoPageSize is enabled, the pageSize prop value should not be used as it should be calculated according to the height of the viewport.

fix #1334

@dtassone dtassone self-assigned this Apr 9, 2021
@dtassone dtassone added bug 🐛 Something doesn't work priority: important This change can make a difference labels Apr 9, 2021
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

I have logged the values of the state the current change leads to in https://codesandbox.io/s/material-demo-forked-q56bh?file=/demo.js:0-803. It logs

virtualRowsCount: 5, // The number of rows allocated for the rendered zone.
viewportPageSize: 5, // The number of rows that fit in the viewport.
renderingZonePageSize: 5, // The maximum number of rows that will be rendered at any given time in the grid.

but from how I understand the description of the state, we should have:

virtualRowsCount: 4, // The number of rows allocated for the rendered zone.
viewportPageSize: 5, // The number of rows that fit in the viewport.
renderingZonePageSize: 4, // The maximum number of rows that will be rendered at any given time in the grid.

The proposal done in #1334 (comment) goes in this direction. Considering the previous exploration done, the minimum I would have expected is the pull request description to explain why the previous proposal did wasn't flying. Instead, mentions to it. Could you expand on what are the expected values in the state? Is the current logic meant to support autoPageSize + autoHeight?

@oliviertassinari oliviertassinari changed the title [DataGrid] fix autoPagesize [DataGrid] fix autoPageSize Apr 9, 2021
@oliviertassinari oliviertassinari changed the title [DataGrid] fix autoPageSize [DataGrid] Fix autoPageSize Apr 9, 2021
@dtassone
Copy link
Member Author

dtassone commented Apr 12, 2021

it's actually

virtualRowsCount: 5, // The number of rows that are rendered at once. If pagination then it would be page size. If not, it would be the full set of rows. 
viewportPageSize: 5, // The number of rows that fit in the viewport.
renderingZonePageSize: 5, // The number of rows that fit in the rendering zone.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It's almost good to go.

it's actually

Ok. Could we update the documentation for each of these states? It took what was documentation and tried to find a solution based on staying true to them. e.g.
https://github.com/mui-org/material-ui-x/blob/575f10a24a3818580a16a8b6700bd50c1f6efb22/packages/grid/_modules_/grid/models/gridContainerProps.ts#L39-L42

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Great

@dtassone dtassone merged commit 8dad46e into mui:master Apr 13, 2021
@dtassone dtassone deleted the fixAutoRender branch April 13, 2021 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work priority: important This change can make a difference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DataGrid] autoPageSize does not work anymore
3 participants