-
-
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
[DataGridPro] Autosize columns #10180
Conversation
Netlify deploy previewNetlify deploy preview: https://deploy-preview-10180--material-ui-x.netlify.app/ Updated pages
These are the results for the performance tests:
|
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx
Outdated
Show resolved
Hide resolved
I've addressed all the comments, if you want to review again go ahead, otherwise this is ready for release AFAICT. |
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.
Great value addition to the MUI X datagrid. 🎉
Thank you @romgrk 🙏
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 is an exciting feature! I love the trick of disabling virtualization to measure, smart.
I have tried to use it in https://deploy-preview-10180--material-ui-x.netlify.app/x/react-data-grid/#pro-plan but there is a bug:
Screen.Recording.2023-09-20.at.00.26.34.mov
/** | ||
* Basic statistical outlier detection, checks if the value is `F * IQR` away from | ||
* the Q1 and Q3 boundaries. IQR: interquartile range. | ||
*/ | ||
function excludeOutliers(inputValues: number[], factor: number) { | ||
if (inputValues.length < 4) { | ||
return inputValues; | ||
} | ||
|
||
const values = inputValues.slice(); | ||
values.sort((a, b) => a - b); | ||
|
||
const q1 = values[Math.floor(values.length * 0.25)]; | ||
const q3 = values[Math.floor(values.length * 0.75) - 1]; | ||
const iqr = q3 - q1; | ||
|
||
// We make a small adjustment if `iqr < 5` for the cases where the IQR is | ||
// very small (e.g. zero) due to very close by values in the input data. | ||
// Otherwise, with an IQR of `0`, anything outside that would be considered | ||
// an outlier, but it makes more sense visually to allow for this 5px variance | ||
// rather than showing a cropped cell. | ||
const deviation = iqr < 5 ? 5 : iqr * factor; | ||
|
||
return values.filter((v) => v > q1 - deviation && v < q3 + deviation); | ||
} |
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 have some doubts about the configuration of this behavior with outliersFactor
. Did we consider excluding rows if their width is larger than Q3 + a max outlier distance variable in px? It feels like an easier variable for designer to tweak, it feels closer to the UX tradeoff Product Designers will need to make.
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.
An absolute value isn't adaptive enough for outliers detection in the general case, but it might work for this particular case due to the widths being pixel values. But I'm not totally convinced, most of the time outliers detection with the standard formula works for the huge majority of use cases. And the thing is, if users are at the point where they're configuring pixels variance values, then they already have a good idea of the approximate size of their cells, and they'd probably just set a width directly.
I think this is good enough for now, and if we ever have a request for that type of configuration we can trivially add an outliersDeviation
option that would supersede outliersFactor
if present. It would be non-breaking so it can be released quickly if there is a need for 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.
@romgrk As a product designer, I suspect that the smaller the Q3 width is, the more space there is on the screen, the biggest the breakpoint can be tolerated since there is more free space on the screen. And the bigger the Q3 width is, the less space there is on the screen, the smaller the breakpoint the better.
So do I want the breakpoint to be at Q3 width + 200px
, or Q3 + (Q3 - Q1 width) * 1.5
? I think Q3 width + 200px
is better, it better approximates the above goal, and it's simpler to reason with.
As a side note, I don't understand why Q1 is in the formula. We have an optimization problem here, using the least amount of column width while seeing the maximum amount of information, so it seems to all be about the p50, p75, and p95, anything before these percentiles seems not relevant.
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.
As a side note, I don't understand why Q1 is in the formula. We have an optimization problem here, using the least amount of column width while seeing the maximum amount of information
I used the standard stats outlier detection formula: https://www.statisticshowto.com/statistics-basics/find-outliers. It's used for a very wide range of applications and usually "just works".
I'm not sure I'd trust an absolute number. Let's say we take Q3 + 200px
, what if most of the widths are in the [50, 60]
range? A cell with a width of 150px
is 3 times that, and wouldn't be detected as an outlier. The IQR (and by extension Q1, which is in there) is useful because it makes it more adaptive to the data.
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.
what if most of the widths are in the [50, 60] range? A cell with a width of 150px is 3 times that, and wouldn't be detected as an outlier.
IMHO this case doesn't have be considered an outlier. This matches with the default behavior of Excel and Google Sheets which resizes including outliers.
I have found an issue related to this: #10456
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/gridColumnResizeApi.ts
Outdated
Show resolved
Hide resolved
packages/grid/x-data-grid-pro/src/hooks/features/columnResize/gridColumnResizeApi.ts
Outdated
Show resolved
Hide resolved
::: | ||
|
||
:::warning | ||
The datagrid can only autosize based on the currently rendered cells. |
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.
It should be at minimum two words to be coherent with <DataGrid>
and @mui/x-data-grid
and https://www.notion.so/mui-org/Style-conventions-for-MUI-components-and-products-dff7d2d7d1ba4a4abd47f48cf345b800?pvs=4#8f86b8e3c0fb49e1b4df3276efb3848e.
The datagrid can only autosize based on the currently rendered cells. | |
The data grid can only autosize based on the currently rendered cells. |
x the other cases.
However, I suspect Sam would push for as it references our data grid, not data grids in general.
The datagrid can only autosize based on the currently rendered cells. | |
The Data Grid can only autosize based on the currently rendered cells. |
but this hasn't been applied very consistently so far, so unclear.
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'll switch to data grid
for now, but just to note I don't love data grid
and even less Data Grid
because it can be confused for a variant of Grid. I'd prefer datagrid
or DataGrid
to any of those.
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.
To illustrate the move towards Data Grid: https://github.com/mui/mui-x/pull/5781/files, which is equivalent to the normalization ongoing in the core by @samuelsycamore.
History, grid would intuitively mean data gird, e.g. https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/data-grids/ uses this a bit interchangeably.
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 would strongly prefer to see Data Grid capitalized to emphasize that we're talking about the product, not the general concept. DataGrid
is an outdated convention that we're slowly trying to phase out of the docs. If you need to make it clear that you're talking about the code specifically, use <DataGrid />
. Overall it's less important what we use so long as we apply it consistently, and that's why I try to enforce the conventions that I've outlined in the Notion doc linked above.
…gridColumnResizeApi.ts Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Rom Grk <[email protected]>
…gridColumnResizeApi.ts Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Rom Grk <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Rom Grk <[email protected]>
Co-authored-by: Olivier Tassinari <[email protected]> Signed-off-by: Rom Grk <[email protected]>
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
I think this is quite a basic functionality that should've been included from the start, and there is lots of valid feedback in #1241 regarding how the useability of the DataGrid component is compromised without this fix. The experience of DataGrid with some data sets can be quite poor right now, especially on mobile. This fix goes a long way towards improving that, so while it is encouraging to see that it is being worked on, it is very concerning that this could be a 'pro plan only' fix. Locking such an essential fix behind a subscription-based plan will create a large divide regarding the experience and useability of the DataGrid component between the standard and pro plans. |
@romgrk I feel column resize and column autosizing now deserve separate pages under |
@sphinx9 I have created #7323 about this problem. It includes one workaround #1040 (comment) which could make a great "receip" in the docs. Column resizing helps but I wouldn't expect it to be the main solution though. Resizing columns can be painful. You raised an interesting point though on how feature split is determined. One could see Pro around handling more data, Premium around analyzing a lot of data and Community around being the fastest and simplest way to display small data sets.
@MBilalShafi I like how it would better isolate community content, make it feel more like something that can be used on its own.
I was wondering if this autosize feature should be available on mobile, as a double tap, but I couldn't find any data grid that does this so I assume it's probably best "pointer: fine" only. A side thought: in https://mui.com/x/react-data-grid/column-dimensions/#resizing. It makes me think of https://mui.com/x/react-data-grid/virtualization/#column-virtualization we could have the notion of Edit: ah, it already exists, great, it just that it's not in the demo, so maybe to document 😁. |
@@ -58,6 +58,45 @@ To capture changes in the width of a column there are two callbacks that are cal | |||
- `onColumnResize`: Called while a column is being resized. | |||
- `onColumnWidthChange`: Called after the width of a column is changed, but not during resizing. | |||
|
|||
## Autosizing [<span class="plan-pro"></span>](/x/introduction/licensing/#pro-plan 'Pro plan') | |||
|
|||
`DataGridPro` allows to autosize the columns' dimensions based on their content. Autosizing is enabled by default. To turn it off, pass the `disableAutosize` prop to the datagrid. |
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.
Autosizing is enabled by default.
This is a bit misleading since it could be understood as "columns are autosized by default", which is not the case.
Should we rephrase it to "Double-clicking a column header separator is enabled by default"?
@romgrk
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.
Sure, that sounds good.
|
||
Autosizing can be used by one of the following methods: | ||
|
||
- Adding the `autosizeOnMount` prop, |
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.
Nitpick: We could be more explicit when naming props. For example autosizeColumnsOnMount
, disableColumnsAutosize
and columnsAutosizeOptions
I am finding this to not work on mobile size screens. My column headers are still being cut off (this is also reproducible in the example on the docs currently)
|
@DevPowers Could you open a new issue with the link to the docs example you're referring to? Please also clarify what is not working (double-click or autosize-on-mount). |
Closes #1241
Implement columns auto-sizing.
Current plan
API
Single API method:
autosizeColumns
By default, applies on all columns. If
options.columns
is specified, only apply to those instead.Implementation
For performance reasons, notably to avoid introducing a wrapping component around each cell, I think the easiest way to implement this is to access the DOM directly. I think this also simplifies the implementation a lot.
Preview: https://deploy-preview-10180--material-ui-x.netlify.app/x/react-data-grid/column-dimensions/#autosizing