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

[data grid] Column resize fit not fitting the content of the cells #10456

Open
oliviertassinari opened this issue Sep 24, 2023 · 4 comments
Open
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer feature: Column resize

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 24, 2023

Steps to reproduce 🕹

Link to live example:

Steps:

  1. Open https://codesandbox.io/s/peaceful-blackwell-ys6dc6?file=/Demo.tsx
Screen.Recording.2023-09-24.at.18.38.07.mov

Current behavior 😯

"Partially Filled" is considered an outlier width and ignored in the default resize algorithm.

Expected behavior 🤔

"Partially Filled" is not considered an outlier.

Context 🔦

Found this from #10180 (comment). It feels like the outlier detection logic is too sensitive to small width differences.

Your environment 🌎

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Side notes

In this demo, we can also notice two things:

  1. const contentWidth = (cell.firstElementChild?.scrollWidth ?? -1) + 1;
    is not taking into account the element border. So when we resize, it's 2px too short. To consider if .getBoundingClientRect().width would be relevant.
Screenshot 2023-09-24 at 19 33 28
  1. Resizing this column to be smaller, and then double-clicking doesn't work as the previous element never expands to remove the ellipses. To consider if we https://github.com/ag-grid/ag-grid/blob/9333ca29347c9bb4d182ebcc15150ef7676e917c/grid-community-modules/core/src/ts/rendering/autoWidthCalculator.ts#L55 makes sense (MIT module)
@oliviertassinari oliviertassinari added component: data grid This is the name of the generic UI component, not the React module! status: waiting for maintainer These issues haven't been looked at yet by a maintainer plan: Pro Impact at least one Pro user labels Sep 24, 2023
@oliviertassinari oliviertassinari added the design This is about UI or UX design, please involve a designer label Sep 24, 2023
@MBilalShafi MBilalShafi added feature: Column resize bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 25, 2023
@romgrk
Copy link
Contributor

romgrk commented Sep 25, 2023

1. Outlier detection

I think one problem we have is that our sample size is small. If we were to autosize at a different part of the grid, that value would not be considered an outlier anymore:

I'm not sure how to settle this one. I still feel like the current formula is going to work better for a wider range of cases. We can always find in-between cases where it gives worse results than a different formula, but it's not really clear cut. I think for this particular example, it looks particularly off because we know what the rendered widgets look like and we'd expect the grid to fit them perfectly, but imagine that same column just as text. Would it look as off if it was pure text?

I also think the tradeoffs between wasted space & information display are going to vary depending on the data itself. The more importance a column has, the less acceptable it would be to the user to crop outliers.

I think maybe we could explore making autosizing configurable by column? With some columns accepting different factors/formulas? I feel that this is a bit early though, if we could collect a bit more feedback we might have a clearer answer.

2. Missing a few pixels

Yeah I did it with .getBoundingClientRect initially but we decided to switch to .scrollWidth for performance reasons. We'll switch back to the first implementation in light of this information.

Discussion: #10180 (comment)

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Oct 8, 2023

I also think the tradeoffs between wasted space & information display are going to vary depending on the data itself. The more importance a column has, the less acceptable it would be to the user to crop outliers.

I think maybe we could explore making autosizing configurable by column? With some columns accepting different factors/formulas? I feel that this is a bit early though, if we could collect a bit more feedback we might have a clearer answer.

@romgrk Right, so if we were to list options:

A. What you referenced above could make sense. In Notion, you can wrap vs. unwrap columns depending on the data (really useful), so why not have the same but for developers (not end-users)

Screenshot 2023-10-08 at 17 58 15

B. In Excel or Google Sheets, I'm not aware there is outlier detection logic when resizing columns: it resizes (though there is a max width). So it could be an opt-in behavior rather than an opt-out like today. I don't feel I would configure this per column but more keep it disabled by default as it feels unpredictable. There, I would likely need a default max-width value, like what's the point of a column that is >2,000px wide? To consider if a max width should be shared with resizing and autolayout or distinct: https://mui.com/x/react-data-grid/column-dimensions/#minimum-width.

C. We could have a less sensitive threshold, but wouldn't really solve the problem here when there is a select type column.

D. We could have the filtering logic based on how many pixels are allowed between p50 and p100. Anchored around #10180 (comment) rationale.

E. We measure more rows, allowing for a more reliable percentile measurement. But even then, it doesn't feel like it would solve the problem. If all the select have the same width, except for one that is 5px larger, current logic would consider it an outlier ❌. I mean, I make the assumption that in such a case Q3 = Q1. and the formula, cut it: Q3 + (Q3 - Q1 width) * 1.5

@oliviertassinari oliviertassinari removed the plan: Pro Impact at least one Pro user label Jul 25, 2024
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jul 25, 2024

Another example where I think this bug is visible: https://olympicsdatagenie.mui.com/

(I double click to column autosize, the macOS built-in screen recorder doesn't show clicks anymore, looks like a regression)

Screen.Recording.2024-07-25.at.12.24.45.mov

It feels like #10180 (comment) is still relevant.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Dec 22, 2024

I have just experienced this on https://tools-private.mui.com/prod/pages/wooCommerceCustomer. Double-click on the "First item" column to autosize.

Screen.Recording.2024-12-22.at.19.48.19.mov

Actual: Shrink to be super small
Expected: Fit all the cells, it's not that big

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! design This is about UI or UX design, please involve a designer feature: Column resize
Projects
None yet
Development

No branches or pull requests

3 participants