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.

Sign up for GitHub

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

Jump to bottom

[DataGrid] Add minWidth to GridColDef #2101

Merged
merged 12 commits into from
Jul 20, 2021
Merged

[DataGrid] Add minWidth to GridColDef #2101

merged 12 commits into from
Jul 20, 2021

Conversation

DanailH
Copy link
Member

@DanailH DanailH commented Jul 8, 2021 •
edited by oliviertassinari
Loading

Fixes #1850

A follow up on this PR -> #1954

The minWidth also works in combination with the column resize feature as in if minWidth is provided the resizing will be restricted and the width of the column cannot be smaller than the set minWidth.

I used the same example, with just a small tweak.

Preview: https://deploy-preview-2101--material-ui-x.netlify.app/components/data-grid/columns/#minimum-width

@DanailH DanailH added component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request labels Jul 8, 2021
@DanailH DanailH requested a review from a team July 8, 2021 12:00
@DanailH DanailH self-assigned this Jul 8, 2021
@DanailH DanailH changed the title Add minWidth to GridColDef [DataGrid] Add minWidth to GridColDef Jul 8, 2021
@DanailH DanailH mentioned this pull request Jul 8, 2021
Closed
2 tasks
@flaviendelangle
Copy link
Member

We should probably update the Resizing documentation with a line describing the minWidth (and in the future the maxWidth) impact on the resize.

And add a minWidth to one of the two resizable columns of the doc example.

oliviertassinari
oliviertassinari previously requested changes Jul 9, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case where the new logic doesn't work https://codesandbox.io/s/material-demo-forked-hkgdo?file=/demo.js. There is enough space not to have a scrollbar, and yet we display one.

packages/grid/_modules_/grid/models/colDef/gridColDef.ts Show resolved Hide resolved
packages/grid/_modules_/grid/models/colDef/gridColDef.ts Outdated Show resolved Hide resolved
packages/grid/data-grid/src/tests/layout.DataGrid.test.tsx Outdated Show resolved Hide resolved
packages/grid/_modules_/grid/components/columnHeaders/GridColumnHeaderItem.tsx Outdated Show resolved Hide resolved
DanailH and others added 6 commits July 9, 2021 15:01
@DanailH @oliviertassinari
Update packages/grid/_modules_/grid/models/colDef/gridColDef.ts
74cdccc 
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@DanailH @oliviertassinari
Update packages/grid/data-grid/src/tests/layout.DataGrid.test.tsx
ca65a71 
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@DanailH
revert minWidth in cells
005e577
@DanailH
Merge branch 'feature/DataGrid-1850-add-minWidth-in-ColDef' of github…
625486e 
….com:DanailH/material-ui-x into feature/DataGrid-1850-add-minWidth-in-ColDef

:Please enter a commit message to explain why this merge is necessary,
@DanailH
Pr comments
94ec04d
@DanailH
update docs
3c0082e
m4theushw
m4theushw reviewed Jul 13, 2021
View reviewed changes
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Open https://codesandbox.io/s/material-demo-forked-8soge?file=/demo.js and note that the age column is hidden. I think we forgot to consider minWidth when computing the available viewport width:

https://github.com/mui-org/material-ui-x/blob/3c0082e6ae06790fde25e4f2772b892162ffaa89/packages/grid/_modules_/grid/hooks/features/columns/useGridColumns.ts#L46

packages/grid/_modules_/grid/hooks/features/columns/useGridColumns.ts Outdated Show resolved Hide resolved
packages/grid/_modules_/grid/components/cell/GridRowCells.tsx Outdated Show resolved Hide resolved
@DanailH DanailH requested a review from m4theushw July 20, 2021 08:20
@DanailH
Copy link
Member Author

DanailH commented Jul 20, 2021

Ok, I approved argos - it was complaining because of the 50px minWidth. It would be great if I can merge it 😅

oliviertassinari
oliviertassinari reviewed Jul 20, 2021
View reviewed changes
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to not have the logic leak outside of the two relevant hooks 👍

docs/src/pages/components/data-grid/columns/columns.md Outdated Show resolved Hide resolved
docs/src/pages/components/data-grid/columns/columns.md Outdated Show resolved Hide resolved
@DanailH DanailH mentioned this pull request Jul 20, 2021
Closed
1 task
@DanailH
Copy link
Member Author

DanailH commented Jul 20, 2021

Created a follow up ticket for maxWidth -> #2174

@DanailH DanailH merged commit d0f19e1 into mui:master Jul 20, 2021
@flaviendelangle flaviendelangle mentioned this pull request Jul 21, 2021
Closed
@oliviertassinari oliviertassinari mentioned this pull request Nov 12, 2021
Open
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@dtassone dtassone dtassone approved these changes

@flaviendelangle flaviendelangle Awaiting requested review from flaviendelangle

@m4theushw m4theushw Awaiting requested review from m4theushw

Assignees

@DanailH DanailH

Labels
component: data grid This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[DataGrid] Introduces minWidth in ColDef for columns with fluid width
5 participants
@DanailH @flaviendelangle @dtassone @oliviertassinari @m4theushw