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] Remove headerHeight prop #602

Closed
Tracked by #3287
oliviertassinari opened this issue Nov 18, 2020 · 5 comments · Fixed by #7001
Closed
Tracked by #3287

[DataGrid] Remove headerHeight prop #602

oliviertassinari opened this issue Nov 18, 2020 · 5 comments · Fixed by #7001
Assignees
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! v6.x waiting for 👍 Waiting for upvotes

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 18, 2020

The grid has a headerHeight that we should be able to remove by moving the AutoSizer down in the DOM tree, to only where we need it: for the rendering zone. It would solve this confusion: #597 (comment) and this bug: #597 (review). It's also the approach used by AG Grid.

@cherniavskii
Copy link
Member

AutoSizer has been moved down in the DOM tree and it doesn't contain the columns header anymore:
Screenshot 2022-11-23 at 15 09 07

headerHeight prop is still being used:

  1. By virtual scroller to add a top margin and not overlap with columns header (columns header has position absolute
    height: size.height ? size.height - totalHeaderHeight : 'auto',
    marginTop: totalHeaderHeight,
  2. For density, since it impacts the columns header height as well
    case GridDensityTypes.Compact:
    return {
    value: newDensity,
    headerHeight: Math.floor(newHeaderHeight * COMPACT_DENSITY_FACTOR),
    rowHeight: Math.floor(newRowHeight * COMPACT_DENSITY_FACTOR),
  3. For column header grouping, since the same height is used for each "row" of column headers

    And some more places.

I don't see any reason to remove the headerHeight prop.


We can address the confusion raised in #597 (comment) by removing the Header slot:

/**
* Header component rendered above the grid column header bar.
* Prefer using the `Toolbar` slot. You should never need to use this slot.
* @default GridHeader
*/
Header: React.JSXElementConstructor<any>; // TODO remove.

I've created a separate issue for it #6977.

@cherniavskii
Copy link
Member

Feel free to point out any other issues related to the headerHeight prop.
Otherwise, we can close the issue.

@m4theushw
Copy link
Member

I don't remember the old position of the AutoSizer but the problem raised by @oliviertassinari remains. It always measures its parent and, by being inside GridMainContainer, the measurements will correspond to the entire grid. Because it doesn't measure only the content portion we need to subtract the column headers height, so headerHeight comes in place to obtain only the height available for the content. If the column headers weren't absolute positioned, the marginTop (1.) applied to the virtual scroller could be removed. flex: 1 could be used in places where we manually set the height, actually what we really want is "use the available height". For 2., since developers can't change the factor, we can add CSS classes with the fixed header height for each density. Another option is to apply vertical padding to the column headers to simulate the "standard" and "comfort" density, because "compact" would become the default (=no padding). In 3., CSS can also be used to set the column grouping header height.

To summarize, the idea is to fix the AutoSizer so it measures only the content portion. As result, the headerHeight prop will become useless because we won't need to use it to subtract from the grid height.

@cherniavskii
Copy link
Member

@m4theushw Thanks for looking into it!

If the column headers weren't absolute positioned, the marginTop (1.) applied to the virtual scroller could be removed.

I tried to do this in #7001
It seemed to be very promising at first, but I wasn't able to fix regressions.
Turns out column headers have position: 'absolute' for a reason. Without it, the grid exceeds the width of the parent container instead of showing a horizontal scroll in the flex layout:
https://app.argos-ci.com/mui/mui-x/builds/6190/29684941

Screenshot 2022-11-25 at 19 49 17

To summarize, the idea is to fix the AutoSizer so it measures only the content portion. As result, the headerHeight prop will become useless because we won't need to use it to subtract from the grid height.

I think doing that would require a lot of time, while the gain would be minimal.

Furthermore, I find the headerHeight prop convenient since it allows changing row and header height in the same way.
If we remove it - it won't be obvious that the header height should be changed with CSS instead.

What do you think?

@m4theushw
Copy link
Member

I think doing that would require a lot of time, while the gain would be minimal.

I think that in the beginning there was only headerHeight and rowHeight, but now we have other heights (e.g. footer height and column grouping header height) so keeping it makes sense for consistency. Maybe we rename it to columnHeaderHeight as suggested in #597 (comment). Besides that, by only fixing the GridAutoSizer it will substantially help us, because now we'll just need to connect the prop to the style of the column headers, without additional calculations. I think most of the gain is here since the tests can be simplified.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module! v6.x waiting for 👍 Waiting for upvotes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants