-
-
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 horizontal scroll not working on empty grid #3821
Conversation
These are the results for the performance tests:
|
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.
Could you add tests for this part? We should cover more the virtualization.
This comment was marked as resolved.
This comment was marked as resolved.
I've added a test to check virtualScrollContent height. |
setShouldExtendContent( | ||
apiRef.current.windowRef.current.scrollHeight <= | ||
apiRef.current.windowRef.current.clientHeight, | ||
); |
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.
This callback will be called when the virtualScrollerContentSizeChange
event is fired and the event is fired when contentSize
changes, which happens inside the hook. We can remove this state update and migrate the minHeight
to contentSize
, avoiding one rerender.
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.
Nice catch!
I noticed that the event is emitted in the same hook, but didn't think of removing unnecessary state
packages/grid/x-data-grid-pro/src/DataGridProVirtualScroller.tsx
Outdated
Show resolved
Hide resolved
packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx
Outdated
Show resolved
Hide resolved
@@ -320,12 +320,19 @@ export const useGridVirtualScroller = (props: UseGridVirtualScrollerProps) => { | |||
const needsHorizontalScrollbar = containerWidth && columnsMeta.totalWidth > containerWidth; | |||
|
|||
const contentSize = React.useMemo(() => { | |||
let shouldExtendContent = false; | |||
const windowRef = apiRef.current.windowRef; | |||
if (windowRef?.current && windowRef.current.scrollHeight <= windowRef.current.clientHeight) { |
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.
Note that windowRef
is a wrapper for the .MuiDataGrid-virtualScrollerContent
element so windowRef.current.scrollHeight
is equal to size.height
.
if (windowRef?.current && windowRef.current.scrollHeight <= windowRef.current.clientHeight) { | |
if (windowRef?.current && size.height <= windowRef.current.clientHeight) { |
Use ref
instead of windowRef
. They are the same thing:
mui-x/packages/grid/_modules_/grid/components/base/GridBody.tsx
Lines 92 to 93 in b12b22b
<VirtualScrollerComponent | |
ref={windowRef} |
mui-x/packages/grid/x-data-grid/src/DataGridVirtualScroller.tsx
Lines 15 to 16 in b12b22b
const { getRootProps, getContentProps, getRenderZoneProps, getRows } = useGridVirtualScroller({ | |
ref, |
mui-x/packages/grid/_modules_/grid/hooks/features/virtualization/useGridVirtualScroller.tsx
Lines 372 to 373 in b12b22b
getRootProps: ({ style = {}, ...other } = {}) => ({ | |
ref: handleRef, |
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've used rootRef
instead of ref
, since ref
has type React.Ref<HTMLDivElement>
, which allows it to be a function and I wasn't able to just get .current
from it.
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 good.
Co-authored-by: Matheus Wichman <matheushw@outlook.com>
Fixes #3795 (comment)