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 dimensions computation with autoHeight and scroll x #5401

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jul 5, 2022

Fixes #5398

The problem was that when props.autoHeight = true, viewportOuterSize.height === content.height
If there is a scrollbar for the X axis, then the viewportOuterSize.height can't fit the content AND the scrollbar for the X axis.
So the dimensions script thinks he must put a scrollbar for the Y axis (which will never be rendered since #5164)

As a consequence, the pinned columns think they must offset by the width of the scroll bar in the Y axis even if it's not needed.


My solution is first to make sure that there is never a scrollar for the Y axis in the rooter dimensions when props.autoHeight = true.
And then to fix the viewportOuterSize.height to take the scrollbar for the X axis into account if any.

Codesandbox of #5398 with the fix

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
@flaviendelangle flaviendelangle self-assigned this Jul 5, 2022
@flaviendelangle flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! regression A bug, but worse labels Jul 5, 2022
@mui-bot
Copy link

mui-bot commented Jul 5, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 264.5 465.4 363.4 348.8 71.139
Sort 100k rows ms 462.9 834 696.1 678.36 119.932
Select 100k rows ms 130.2 187.7 149.2 156.88 24.339
Deselect 100k rows ms 101.1 221.1 204.2 170.6 49.766

Generated by 🚫 dangerJS against 2f99cb1

@flaviendelangle flaviendelangle marked this pull request as ready for review July 5, 2022 13:06

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
let hasScrollX: boolean;
let hasScrollY: boolean;

if (props.autoHeight) {
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 split the logic in two because we started to have a lot of conditions with autoHeight inside the logic, making it harder to read.

When autoHeight is on, the logic is very simple since we only have to care about one direction.

Copy link
Member

@cherniavskii cherniavskii left a comment

Choose a reason for hiding this comment

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

Looks great!

@flaviendelangle flaviendelangle merged commit d38836f into mui:master Jul 6, 2022
@flaviendelangle flaviendelangle deleted the scrollbar-autoheight branch July 6, 2022 07:35
joserodolfofreitas pushed a commit to joserodolfofreitas/mui-x that referenced this pull request Jul 15, 2022

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022

Unverified

This commit is not signed, but one or more authors requires that any commit attributed to them is signed.
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! regression A bug, but worse
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Datagrid] AutoHeight + pinned right column = column header misplaced
3 participants