-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[DataGrid] Fix dimensions computation with autoHeight and scroll x #5401
Conversation
These are the results for the performance tests:
|
657d2a6
to
2f99cb1
Compare
let hasScrollX: boolean; | ||
let hasScrollY: boolean; | ||
|
||
if (props.autoHeight) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
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