[DataGrid] Fix autoHeight computation for custom headers and footers

[DanailH]

Fixes #502

This PR aims to fix an issue that occurs when the autoHeight prop is set and a custom header and/or footer is provided. The bug is that the height of the grid, in that case, is not calculated correctly. That results in the last rows being hidden.

[oliviertassinari]

I think that we could have pushed the solution deeper. This is a partial solution. Consider the following case: https://codesandbox.io/s/cool-thunder-olrqk?file=/demo.js.

From my perspective, the best solution would be to move the AutoSizer to the grid zone.

[oliviertassinari]

autoHeight={false} doesn't seem impacted (I don't know why), so happy to call it good enough.

https://codesandbox.io/s/romantic-mestorf-zw1vg?file=/demo.js

[DanailH]

I think that we could have pushed the solution deeper. This is a partial solution. Consider the following case: https://codesandbox.io/s/cool-thunder-olrqk?file=/demo.js.

From my perspective, the best solution would be to move the AutoSizer to the grid zone.

I explored that solution as well. It can be done but it involves removing outerStyle.height = 0; and outerStyle.width = 0; from the AutoSizer to give the grid the correct dimensions. If I don't do it and I move the header and footer outside of the AutoSizer they collapse because the grid outer container has a height of 0. This btw maybe another bug that I am happy to solve but I don't know if anything will be impacting my removing those. When I tested it I didn't find any visible regressions. Maybe I can open another PR with the other solution and see if something will fail?

[oliviertassinari]

1. I'm getting confused with the headerHeight prop, it seems to be a columnHeaderHeight prop after all, completely unrelated to the "header" component. Maybe we need to rename it? I start to think that my latest suggestion was wrong (4b12631). cc @dtassone
2. The AutoSizer should be left untouched, the logic is correct, it needs to be height = 0 and width = 0 to remove the feedback loop. Here is what I was thinking, move the logic closer where we need it, it's for the virtualization and only:

diff --git a/packages/grid/_modules_/grid/GridComponent.tsx b/packages/grid/_modules_/grid/GridComponent.tsx
index 5e88c334..cc4a6bd2 100644
--- a/packages/grid/_modules_/grid/GridComponent.tsx
+++ b/packages/grid/_modules_/grid/GridComponent.tsx
@@ -103,8 +103,6 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps
     );

     return (
-      <AutoSizer onResize={onResize}>
-        {(size: any) => (
           <GridRoot
             ref={handleRef}
             className={props.className}
@@ -152,6 +150,8 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps
                       gridState.rows.totalRowCount === 0 &&
                       customComponents.noRowsComponent}
                     {props.loading && customComponents.loadingComponent}
+                    <AutoSizer onResize={onResize}>
+                      {(size: any) => (
                     <GridWindow ref={windowRef}>
                       <GridDataContainer
                         ref={gridRef}
@@ -168,6 +168,8 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps
                         )}
                       </GridDataContainer>
                     </GridWindow>
+                      )}
+                    </AutoSizer>
                   </div>
                   {customComponents.footerComponent || (
                     <DefaultFooter
@@ -194,8 +196,6 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps
               </ApiContext.Provider>
             </ErrorBoundary>
           </GridRoot>
-        )}
-      </AutoSizer>
     );
   },
 );

But if it's too much work, we can call the current solution an approximation, it's fine.

[DanailH]

unfortunately, I can't move it that far down. If I do other components also need tweaking (tried doing it and the ScrollArea starts to complain). I was thinking of having something like this:

But that brings other problems. The current solution works. Let's keep it for now and see if people find more problems with this we can spend a bit more time to rework the layout.

[oliviertassinari]

For point 1. #597 (comment), so we don't forget about it.

[oliviertassinari]

@DanailH Agree for 2. it would require a stronger refactorization. We can wait for developers complaining about the limitation before spending more time. This is the most pragmatic approach, I think.

[DanailH]

For point 1. #597 (comment), so we don't forget about it.

Updated the PR - we still need that prop as it is the default header height (not the columns header).

[oliviertassinari]

@dtassone What do you think about we rename headerHeight to columnHeaderHeight? Should we open a new issue about it? Right now, it could be confused with the height of the header component. Only the description clears the confusion:

Set the height in pixel of the column headers in the grid.

[DanailH]

@dtassone can I merge this ?

[dtassone]

For the headerHeight, it's like a row height so, it is the columnCellHeight or columnHeaderCellHeight
headerHeight might be confusing now.

[oliviertassinari]

@dtassone I wonder if we can't even drop this headerHeight prop. It seems to be only relevant for the AutoSizer component. If we move the AutoSizer inside the rendering-zone only, (excluding the header, header column, footer), we should be able to remove the prop. As far as I understand it.