[DataGrid] Move column resizing to XGrid only #257

dtassone · 2020-09-09T17:14:37Z

oliviertassinari

We need to update the documentation. I think that we should also add a custom prop type for when a developer tries to enable the feature on the DataGrid (see the example with pageSize).

oliviertassinari · 2020-09-09T18:28:46Z

I'm working on the DataGrid vs XGrid feature table comparison in #255, I'm going to account for it.

dtassone · 2020-09-10T08:44:28Z

We need to update the documentation. I think that we should also add a custom prop type for when a developer tries to enable the feature on the DataGrid (see the example with pageSize).

Not sure what you mean

oliviertassinari · 2020-09-10T10:03:32Z

We need to update the documentation.

Probably change this section to talk about how to enable column resizing (XGrid) and how to disable it for one column.

https://deploy-preview-257--material-ui-x.netlify.app/components/data-grid/columns/#column-sizing

(see the example with pageSize)

/~https://github.com/mui-org/material-ui-x/blob/8651b355b965d9a20b55340bc25e7ca733096048/packages/grid/data-grid/src/DataGrid.tsx#L76-L89

dtassone · 2020-09-10T13:07:19Z

We need to update the documentation.

Probably change this section to talk about how to enable column resizing (XGrid) and how to disable it for one column.

https://deploy-preview-257--material-ui-x.netlify.app/components/data-grid/columns/#column-sizing

(see the example with pageSize)

/~https://github.com/mui-org/material-ui-x/blob/8651b355b965d9a20b55340bc25e7ca733096048/packages/grid/data-grid/src/DataGrid.tsx#L76-L89

I see! So you prefer to assign them to type true and add proptypes than removing the prop of the options, right?

Any particular reason you check pagesize in disableMultipleColumnSorting?
/~https://github.com/mui-org/material-ui-x/blob/8651b355b965d9a20b55340bc25e7ca733096048/packages/grid/data-grid/src/DataGrid.tsx#L63

oliviertassinari · 2020-09-10T15:19:59Z

Any particular reason you check pagesize in disableMultipleColumnSorting?

It's a mistake from my end, a wrong copy and pastes, we need to fix it :)

oliviertassinari · 2020-09-10T15:23:29Z

I see! So you prefer to assign them to type true and add proptypes than removing the prop of the options, right?

I'm not sure to understand your question. I think that developers should be able to swap DataGrid with XGrid with as little effort as possible.

dtassone · 2020-09-10T15:25:31Z

Any particular reason you check pagesize in disableMultipleColumnSorting?

It's a mistake from my end, a wrong copy and pastes, we need to fix it :)

I fixed it! No worries!

dtassone · 2020-09-10T15:30:37Z

I see! So you prefer to assign them to type true and add proptypes than removing the prop of the options, right?

I'm not sure to understand your question. I think that developers should be able to swap DataGrid with XGrid with as little effort as possible.

yes but as these properties are overwritten, and are not required. I originally remove them from the DataGrid props.
Now the user can use them but only set them to one value, and they are also overwritten down the line.
My first feeling was to not expose those props in DataGrid, but just in XGrid as they can be used properly.

oliviertassinari · 2020-09-10T15:36:15Z

@dtassone What about somebody using XGrid with <XGrid pagination /> and want to move to the DataGrid: <DataGrid pagination />? Should he need to remove the pagination prop? I was assuming that it's extra work and that it could be avoided.

dtassone · 2020-09-10T16:01:42Z

@dtassone What about somebody using XGrid with <XGrid pagination /> and want to move to the DataGrid: <DataGrid pagination />? Should he need to remove the pagination prop? I was assuming that it's extra work and that it could be avoided.

Ahhh good point!

oliviertassinari

A fix of the CI and an update of the documentation and we should be good to go.

oliviertassinari · 2020-09-11T11:10:50Z

packages/grid/x-grid-modules/src/components/styled-wrappers/GridRootStyles.ts

+        '& *::-webkit-scrollbar-track': {
+          borderRadius: 10,
+        },
+        '& *::-webkit-scrollbar': {
+          width: 8,
+          height: 8,
+        },
+        '& *::-webkit-scrollbar-thumb': {
+          borderRadius: 10,
+          backgroundColor: borderColor,
+        },


Wrong pull request? Isn't this one for #260? Could we keep the pull requests isolated :)?

Also, what about we only mess around if the theme is dark? Disturbing the UX as little as necessary. Especially, I don't think that we should change the size of the scrollbar.

docs/src/pages/components/data-grid/columns/ColumnWidthGrid.tsx

packages/grid/data-grid/src/DataGrid.test.tsx

oliviertassinari · 2020-09-11T11:14:18Z

Regarding the docs, what do you think about adding a new header? We have one to disable column resizing per column but not to enable/disable it entirely.

docs/src/pages/components/data-grid/columns/columns.md

packages/grid/x-grid-modules/src/models/gridOptions.tsx

packages/grid/data-grid/src/DataGrid.tsx

packages/grid/data-grid/src/DataGrid.test.tsx

oliviertassinari · 2020-09-11T13:35:50Z

packages/grid/data-grid/src/DataGrid.test.tsx

+  let render;
+  let defaultProps;
+  beforeEach(() => {
+    render = createClientRender();


createClientRender() is already wrapping
a beforeEach, it should stay at the top level.

oliviertassinari · 2020-09-11T13:40:12Z

packages/grid/data-grid/src/DataGrid.test.tsx

+        ).to.throw(
+          [
+            `Material-UI: \` column.resizable = true \` is not a valid prop.`,
+            'Column resizing is not available in the MIT version',


I believe to.throw accepts a partial match. I think only matching with the first sentence will be enough to make sure the error isn't coming from somewhere else, and at the same time, make the test easier to write and less likely to fail the next time we fix a typo or slightly improve the error message.

oliviertassinari · 2020-09-11T13:42:00Z

packages/grid/data-grid/src/DataGrid.tsx

+
+  const hasResizableColumns = columns.some((c) => c.resizable);
+  if (hasResizableColumns) {
+    throw new Error(


What about using a custom prop type? This would allow the code to be removed in production (no bundle size implication). This would also be less disruptive to the DX. This would also give a better component error trace

improve docs

dtassone · 2020-09-11T14:27:03Z

something wrong in the configuration of tsconfig?

Probably due to all the paths in tsconfig, as the npm pkgs are missing

oliviertassinari · 2020-09-11T14:28:07Z

Probably due to all the paths in tsconfig, as the npm pkgs are missing

So it's a regression from the current state of master? After this change, we do no longer make sure TypeScript passes on the Storybook? Which can then lead to wrong examples?

dtassone · 2020-09-11T14:38:39Z

Probably due to all the paths in tsconfig, as the npm pkgs are missing

So it's a regression from the current state of master? After this change, we do no longer make sure TypeScript passes on the Storybook? Which can then lead to wrong examples?

well I added the typecript: {check... as TS was not working on the storybook

oliviertassinari · 2020-09-11T14:46:57Z

well I added the typecript: {check... as TS was not working on the storybook

Oh right, my bad. So it's one step forward :) /~https://github.com/storybookjs/storybook/blob/6c0e00522d1ecbeae6f261fc5eb421238480f3fe/lib/core/src/server/config/defaults.js#L2

dtassone requested a review from oliviertassinari September 9, 2020 17:14

oliviertassinari added bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! and removed bug 🐛 Something doesn't work labels Sep 9, 2020

oliviertassinari changed the title ~~fix #245, disable col resize on DataGrid~~ [DataGrid] Move column resizing to XGrid only Sep 9, 2020

oliviertassinari requested changes Sep 9, 2020

View reviewed changes

dtassone added 2 commits September 10, 2020 17:12

fix mui#245, disable col resize on DataGrid

e8f3ae9

fix proptypes in DataGrid

28e22ed

dtassone force-pushed the issue-245 branch from d3eeec0 to 28e22ed Compare September 10, 2020 15:31

dtassone added 3 commits September 10, 2020 19:05

cleanup resize prop on datagrid colDef

11f4db4

prettier

c643bd8

fix lint

ae1fee5

dtassone requested a review from oliviertassinari September 10, 2020 17:11

fix issue with prettier

46ac11a

oliviertassinari reviewed Sep 10, 2020

View reviewed changes

dtassone added 2 commits September 11, 2020 12:30

finish style and update doc

60991d9

fix resizable type issue

a66dc61

dtassone requested a review from oliviertassinari September 11, 2020 11:08

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

docs/src/pages/components/data-grid/columns/ColumnWidthGrid.tsx Outdated Show resolved Hide resolved

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

docs/src/pages/components/data-grid/columns/columns.md Outdated Show resolved Hide resolved

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

packages/grid/x-grid-modules/src/models/gridOptions.tsx Outdated Show resolved Hide resolved

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

packages/grid/data-grid/src/DataGrid.tsx Outdated Show resolved Hide resolved

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

packages/grid/data-grid/src/DataGrid.test.tsx Outdated Show resolved Hide resolved

dtassone added 2 commits September 11, 2020 14:30

fix PR review

1a849bb

new line

9588b02

dtassone requested a review from oliviertassinari September 11, 2020 12:32

dtassone added 4 commits September 11, 2020 14:41

small fix

ae93a2e

fixing issue with netlify deploy

e935bb9

fix lint

4dcdeb9

check only on dev

e08ba66

oliviertassinari reviewed Sep 11, 2020

View reviewed changes

oliviertassinari and others added 3 commits September 11, 2020 16:14

improve docs

87f159f

Update docs/src/pages/components/data-grid/overview/overview.md

3fcd567

Merge pull request #1 from oliviertassinari/docs-proposal

0cfa8c5

improve docs

fix tests and resizable chec

215f9cf

dtassone requested a review from oliviertassinari September 11, 2020 14:42

small details

f027409

oliviertassinari approved these changes Sep 11, 2020

View reviewed changes

dtassone merged commit 7e4199f into mui:master Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DataGrid] Move column resizing to XGrid only #257

[DataGrid] Move column resizing to XGrid only #257

dtassone commented Sep 9, 2020 •

edited by oliviertassinari

Loading

oliviertassinari left a comment

oliviertassinari commented Sep 9, 2020

dtassone commented Sep 10, 2020

oliviertassinari commented Sep 10, 2020 •

edited

Loading

dtassone commented Sep 10, 2020 •

edited

Loading

oliviertassinari commented Sep 10, 2020

oliviertassinari commented Sep 10, 2020 •

edited

Loading

dtassone commented Sep 10, 2020

dtassone commented Sep 10, 2020

oliviertassinari commented Sep 10, 2020 •

edited

Loading

dtassone commented Sep 10, 2020

oliviertassinari left a comment

oliviertassinari Sep 11, 2020 •

edited

Loading

oliviertassinari Sep 11, 2020 •

edited

Loading

oliviertassinari commented Sep 11, 2020

oliviertassinari Sep 11, 2020

oliviertassinari Sep 11, 2020

oliviertassinari Sep 11, 2020 •

edited

Loading

dtassone commented Sep 11, 2020

oliviertassinari commented Sep 11, 2020 •

edited

Loading

dtassone commented Sep 11, 2020

oliviertassinari commented Sep 11, 2020

[DataGrid] Move column resizing to XGrid only #257

[DataGrid] Move column resizing to XGrid only #257

Conversation

dtassone commented Sep 9, 2020 • edited by oliviertassinari Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari commented Sep 9, 2020

dtassone commented Sep 10, 2020

oliviertassinari commented Sep 10, 2020 • edited Loading

dtassone commented Sep 10, 2020 • edited Loading

oliviertassinari commented Sep 10, 2020

oliviertassinari commented Sep 10, 2020 • edited Loading

dtassone commented Sep 10, 2020

dtassone commented Sep 10, 2020

oliviertassinari commented Sep 10, 2020 • edited Loading

dtassone commented Sep 10, 2020

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Sep 11, 2020

oliviertassinari Sep 11, 2020

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2020

Choose a reason for hiding this comment

oliviertassinari Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

dtassone commented Sep 11, 2020

oliviertassinari commented Sep 11, 2020 • edited Loading

dtassone commented Sep 11, 2020

oliviertassinari commented Sep 11, 2020

dtassone commented Sep 9, 2020 •

edited by oliviertassinari

Loading

oliviertassinari commented Sep 10, 2020 •

edited

Loading

dtassone commented Sep 10, 2020 •

edited

Loading

oliviertassinari commented Sep 10, 2020 •

edited

Loading

oliviertassinari commented Sep 10, 2020 •

edited

Loading

oliviertassinari Sep 11, 2020 •

edited

Loading

oliviertassinari Sep 11, 2020 •

edited

Loading

oliviertassinari Sep 11, 2020 •

edited

Loading

oliviertassinari commented Sep 11, 2020 •

edited

Loading