[DataGrid] Fix fluid columns width when available viewportWidth < 0

[DanailH]

Fixes #1582

Reproduction: https://codesandbox.io/s/material-demo-forked-y2xj0 (when the grid loads start resizing the window to make it small to a point where the last column dissapears)

The problem was that when resizing and space was not enough the fluid columns did not default to the base width of 100px.

Screen.Recording.2021-06-03.at.16.25.05.mov

[oliviertassinari]

Do we have a reproduction, e.g. there is almost no information on the linked issue. What's the issue?

[DanailH]

Do we have a reproduction, e.g. there is almost no information on the linked issue. What's the issue?

I'll add a reproduction but the issue is that if you have only one fluid column (as a base case) and you start resing the grid, there is a point where there is no space for the fluid column to be displayed and the grid doesn't allow scrolling so you cant see the last X columns.

PS: @oliviertassinari I added reproduction

[oliviertassinari]

@DanailH Great thanks for the reproduction 🙏! We can probably rely on a visual test case here.

It seems to be exactly this pain #1561 (comment). In the realm of AG Grid, this would be expected behavior, and they would tell us: add a minWidth property. I personally feel that if we reproduce the flexbox model of CSS with flex, then a very small column width is normal.

[DanailH]

@oliviertassinari Visual test can be tricky in this situation as this only happens when you load the grid and then resize the window so that there isn't enough space for all the columns that already have fixed width. If you load the grid in a small space/window initially the regression doesn't happen.

[oliviertassinari]

Visual test can be tricky in this situation

@DanailH I'm completely lost. I don't understand where the problem is, why visual regression isn't possible, nor what this PR is changing. See the following:

HEAD: The last column is not visible, looks alright ✅, flex: 1 means it should shrink.

https://codesandbox.io/s/material-demo-forked-y2xj0?file=/demo.js:0-587

This PR: Same behavior as on HEAD

https://codesandbox.io/s/material-demo-forked-y8m1q?file=/package.json

[oliviertassinari]

I mean, see how CSS behaves: flex: 1:

And with the same configuration of the columns: https://codepen.io/oliviertassinari/pen/XWMExoz

I wouldn't expect width: 100px here for the age column. I think that it only makes sense if we add minWidth: 100px.

[m4theushw]

@oliviertassinari This PR is fixing the negative widths when there's no available space. A visual test case would not check that but an unit test can.

I think that it only makes sense if we add minWidth: 100px.

So default it to zero?

[oliviertassinari]

@m4theushw I think that it should use the default minWidth value when there is not enough space, not the width value because flex overrides width.

[DanailH]

@m4theushw I think that it should use the default minWidth value when there is not enough space, not the width value because flex overrides width.

@oliviertassinari the thing is that there is no minWidth on the ColDef, I can add one but that is a different PR. Regarding the flex it's just sugar on the ColDef that gets converted to pixels.

[oliviertassinari]

@DanailH Ok, so we can close this PR, close #1582, and open a new standalone issue for #1561 (comment) (solving with a new min-width option)?

The reasoning being:

1. the current logic is correct, it's how CSS work, what we aim to reproduce to avoid developers to learn yet another layout behavior
2. the changes here make the behavior inconsistent:
   

[DanailH]

@oliviertassinari Done, here is the ticket #1850. We can discuss it today during the sprint planning and bring it in.

[oliviertassinari]

By close, I meant close, not merge, the behavior is now inconsistent. But it's fine, we can revert in #1850. Thanks for the new issue.

[DanailH]

By close, I meant close, not merge, the behavior is now inconsistent. But it's fine, we can revert in #1850. Thanks for the new issue.

Sorry I misunderstood. I'll change that once the other ticket is picked up.