[DataGrid] Add Density selector #606

DanailH · 2020-11-19T15:11:42Z

Closes #567

I have added a new property on the DataGrid -> density that can be one of 3 options: compact, standard and comfortable. The default value is standard. In addition, the density property also works with the rowHeight and headerHeight in a way that if those are provided it will take them into account when adjusting the size of the grid.

Density options:

For compact size -> the height of the rows is lowered by 1/3 of their original value.
For comfortable size -> the height of the rows is increased by 1/3 of their original value.

All the logic is contained within the new DensitySelector component.

https://deploy-preview-606--material-ui-x.netlify.app/components/data-grid/rendering/#density

TODO:

Documentation is missing from the PR. Once we agree on the API I'll add the docs and remove the draft status from the PR.
Add unit tests.

oliviertassinari

It looks like a great start.

I don't think that the state approach can scale: 1. the options are overriding by props when the grid rerender with different props and 2. rowHeight can be changed after the first render.
I think that the feature should be opt-in. We might start now to ask the developers to import and inject the component explicitly.
There is a new scrollbar on the header

It could be interesting to use the icon on the Density toolbar button to signal the current state of the density.
@dtassone DataGrid or XGrid?

oliviertassinari · 2020-11-19T15:47:46Z

packages/grid/_modules_/grid/components/DensityPicker.tsx

+  const DensityShortIcon = icons!.densityShort!;
+  const DensityMediumIcon = icons!.densityMedium!;
+  const DensityTallIcon = icons!.densityTall!;


I appreciate the reference to Airtable. In the core components' API, we use the following names: small, medium; large. I would suggest we do the same here

Suggested change

const DensityShortIcon = icons!.densityShort!;

const DensityMediumIcon = icons!.densityMedium!;

const DensityTallIcon = icons!.densityTall!;

const DensitySmallIcon = icons!.densitySmall!;

const DensityMediumIcon = icons!.densityMedium!;

const DensityLargeIcon = icons!.densityLarge!;

cc @mbrookes.

Density is normally high/medium/low (size is small/medium/large)

https://material-density.glitch.me/

vs.

https://material-ui.com/components/tables/#dense-table <Table size="small">

What if we call the prop size?

"density" is more accurate, but if consistency with the Table is important, then sure. (Although I'd be tempted to go the other way, and change table. 😄 )

Is it a prop here though?

It's mostly either margin: dense, or size: small, with one instance of dense: true, and one of variant: dense.

Of the first two, size is closest, even if it doesn't quite describe the purpose.

Ok so I kept the prop name density, should I change it to size and also if yes then the same will apply to the button label right?

I personally like the approach of using:

"density" for the documentation, because it's how Material Design documents it http://material.io/design/layout/applying-density.html or https://medium.com/nextux/design-better-data-tables-4ecc99d23356

"row height", "short", "medium", "tall" for end-users, because it's close to what it does in the UI, at least now, it could change in the future. It's what Airtable does. It will up to the developers to customize it.

"size", "small", "medium", "large" for developers, because it's generic. It's what Chakra does with its components and what we have been doing in the core most recently: https://next.material-ui.com/components/tables/#dense-table.

<Table size="small">

The hook is called useDensity, the component is densityPicker and the label in the toolbar is density so why would we name it size in the prop?
We should call it density as it's less confusing for us as well as for users and also, it's easier to find and understand in the list of props. Button density in the toolbar matches the density prop, it's logical and straightforward.

Otherwise, we can rename everything as useSize, SizePicker, and size in the label, and have a size property.
It doesn't make sense to me to have something named useFoo, Foo and expose it as Bar

The hook is called useDensity, the component is densityPicker and the label in the toolbar is density so why would we name it size in the prop?

We can rename them all to size.

I think that size is superior in the code because it's more generic and allows consistency with the rest of the API. We will need to clean this "mess": https://next.material-ui.com/customization/density/ at some point.

Why do we call <Button size="small"> and not <Button density="dense">? I think that it's because the intrinsic size of the button changes when the prop is applied.
In the case of the data grid, it's a bit different. Because of its structure, the intrinsic size is invariant, unless autoHeight is specified, only the size of the cells changes.

Size seems popular in the other libraries:

https://ant.design/components/table/#components-table-demo-size called size

https://garden.zendesk.com/components/table#size called size

https://www.carbondesignsystem.com/components/data-table/usage called size

https://react-bootstrap.github.io/components/table/#table-small called size

https://elastic.github.io/eui/#/tabular-content/data-grid called size in the class names

https://semantic-ui.com/collections/table.html#size called size

https://material-ui.com/components/tables/#dense-table called size

packages/grid/_modules_/grid/components/DensityPicker.tsx

packages/grid/_modules_/grid/models/gridOptions.tsx

oliviertassinari · 2020-11-19T15:49:34Z

packages/storybook/integration/staticStories.test.ts

@@ -94,6 +94,8 @@ const stories = [
  '/story/x-grid-tests-styling--column-header-class',
  '/story/x-grid-tests-styling--column-cell-class-rules',
  '/story/x-grid-tests-styling--column-cell-renderer',
+
+  '/story/x-grid-tests-toolbar--density-picker',


A side note, we have a great new visual regression runner by @eps1lon in mui/material-ui#23500 :D that we will be able to replace this once we have battle-tested it in the core.

packages/grid/_modules_/grid/components/DensityPicker.tsx

packages/grid/_modules_/grid/components/icons/index.tsx

packages/grid/_modules_/grid/components/DensityPicker.tsx

dtassone · 2020-11-23T11:27:37Z

It could be interesting to use the icon on the Density toolbar button to signal the current state of the density.

Yes I agree, the current density icon should be used in the toolbar

@dtassone DataGrid or XGrid?

XGrid

DanailH · 2020-11-23T12:54:57Z

Ok I checked all the comments and did some additional tests to see how to rework the PR. I'll update the PR with all the requests in bulk. Regarding the state - I think we will need to keep what the current density is in the state. The reason for this is that because the the rowHeight is set via a prop and the density ultimately depends on the rowHeight value.

There is another interesting question tho - if we go with the approach of having an API to change the density then we don't need to expose a prop for the density IMO. Either one or the other because if someone uses both we may and up with weird cases. I took the approach of providing a prop and @dtassone is suggesting an API so we need to decide (after that we will use the same approach for the other toolbar related features)

dtassone · 2020-11-23T13:07:22Z

Ok I checked all the comments and did some additional tests to see how to rework the PR. I'll update the PR with all the requests in bulk. Regarding the state - I think we will need to keep what the current density is in the state. The reason for this is that because the the rowHeight is set via a prop and the density ultimately depends on the rowHeight value.

There is another interesting question tho - if we go with the approach of having an API to change the density then we don't need to expose a prop for the density IMO. Either one or the other because if someone uses both we may and up with weird cases. I took the approach of providing a prop and @dtassone is suggesting an API so we need to decide (after that we will use the same approach for the other toolbar related features)

Things need to be built in a uniform manner.
The apiRef drives the whole grid. It would be great to keep that pattern. Component => hook (update state) => component
Density is like all the other hooks like the sortModel, filters , selection... So you can set a prop, and change it internally with the apiRef, default prop discussion...

…ed state

DanailH · 2020-11-26T11:04:55Z

Ok, it took a but of time but the PR is now ready to be reviewed. The biggest change is that I need to separate the props that the density affects so I created a density section in the state that holds the density size, rowHeight and headerHeight as these are changed when you change the density.

In addition I also added the missing docs and unit tests.

PS: The unit tests that I added are failing because there is a warning in the console (only happens on the tests) that is

Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of Transition which is inside StrictMode.

If someone has any ideas how to solve this I would really appreciate it.

oliviertassinari · 2020-12-03T22:18:06Z

How they affect the density options, whether set through the prop or the menu.

@mbrookes Oh, do you mean for example that the compact density is x% of the rowHeight value?

mbrookes · 2020-12-03T22:27:28Z

Not sure it has to list the specific percentages, but it currently doesn't say that it controls the density. Maybe it doesn't need to? I don't know.

oliviertassinari · 2020-12-03T22:51:45Z

Not sure it has to list the specific percentages, but it currently doesn't say that it controls the density. Maybe it doesn't need to? I don't know.

Isn't it the opposite? density controls the private rowHeight internal state.

mbrookes · 2020-12-04T00:36:44Z

Since I'm talking about the documentation, I'm obviously talking about the prop, not the internal state. 🙂

(Kind of ironic that you said it wasn't confusing†, and are now getting confused by it. 😄 )

oliviertassinari · 2020-12-04T00:46:54Z

Ok, I think I see where you are getting to. You want to document that:

row height when density is compact = rowHeight * 0.7
row height when density is standard = rowHeight * 1
row height when density is confortable = rowHeight * 1.3

mbrookes · 2020-12-04T03:50:29Z

I'm not sure the ratios are too important, just noting the relationship between the prop and density.

Co-authored-by: Matt <github@nospam.33m.co>

…ilH/material-ui-x into feature/DataGrid-567-density-picker

DanailH · 2020-12-04T09:20:22Z

To summarise, @mbrookes . It's not that the rowHeight or the headerHeight values affect the density, it's the other way around. If you mean that by setting custom rowHeight and headerHeight the amount by which the density will increase or decrease will change then yes that is correct but that is also mentioned in the documentation.

oliviertassinari

~~The open animation direction is wrong, it should start from the top, not the bottom.~~ fixed it in [DataGrid] Improve the DX of popups #686
The regression on master was fixed, we can rebase now.

oliviertassinari · 2020-12-04T10:46:09Z

docs/src/pages/components/data-grid/rendering/DensitySelectorHiddenGrid.js

+
+  return (
+    <div style={{ height: 300, width: '100%' }}>
+      <DataGrid {...data} hideToolbar={false} disableDensitySelector />


I think that we should document the opposite, make the toolbar opt-in and document how to render the density toolbar.

import * as React from 'react'; import { DataGrid, GridToolbar, GridToolbarDensitySelector } from '@material-ui/data-grid'; import { useDemoData } from '@material-ui/x-grid-data-generator'; function Header() { return ( <GridToolbar> <GridToolbarDensitySelector /> </GridToolbar> ); } export default function DensitySelectorToolbar() { const { data } = useDemoData({ dataSet: 'Commodity', rowLength: 10, maxColumns: 6, }); return ( <div style={{ height: 400, width: '100%' }}> <DataGrid {...data} components={{ Header }} density="compact" /> </div> ); }

DanailH · 2020-12-04T15:06:15Z

@oliviertassinari so change the hideToolbar to showToolbar ?

oliviertassinari · 2020-12-04T15:32:15Z

so change the hideToolbar to showToolbar ?

@DanailH I wanted to share with @mbrookes a counter-proposal. It's point 7. of #606 (comment). I think that we can handle this once this pull request is merged and before the next release.

To answer your question, it was about the idea to remove the hideToolbar/showToolbar prop.

mbrookes · 2020-12-04T19:34:11Z

If you mean that by setting custom rowHeight and headerHeight the amount by which the density will increase or decrease will change then yes that is correct

Yes

but that is also mentioned in the documentation.

Yes, I know it's mentioned in the documentation, and you know that I know, because we've been discussing the best wording to use. 😀

I was talking about the rowHeight and the headerHeight prop sections. There's no mention currently, but if the toolbar is opt-in, and removed from most demos, it's probably best left for the density section (hence my hesitation).

DanailH · 2020-12-06T11:28:45Z

so change the hideToolbar to showToolbar ?

@DanailH I wanted to share with @mbrookes a counter-proposal. It's point 7. of #606 (comment). I think that we can handle this once this pull request is merged and before the next release.

To answer your question, it was about the idea to remove the hideToolbar/showToolbar prop.

@oliviertassinari I really don't think squeezing this functionality as part of that PR is a good idea. This PR is already to bloated and the composable toolbar is a feature by itself which will bring even more discussions IMO. It is better to merge it as it is (maybe change the hideToolbar to showToolbar) and focus on the composable toolbar.
Also we should aim to do smaller PRs as they are easier to review and test.

oliviertassinari · 2020-12-06T13:00:37Z

@DanailH Agree, this is part of item 7. for another iteration.

oliviertassinari · 2020-12-06T14:31:37Z

The open animation direction is wrong, it should start from the top, not the bottom.

I have fixed it in #686

packages/storybook/src/stories/grid-toolbar.stories.tsx

DanailH · 2020-12-07T14:25:38Z

@dtassone should I merge it ?

dtassone · 2020-12-07T16:03:19Z

docs/src/pages/components/data-grid/rendering/DensitySelectorHiddenGrid.tsx

+
+  return (
+    <div style={{ height: 300, width: '100%' }}>
+      <DataGrid {...data} hideToolbar={false} disableDensitySelector />


hideToolbar={false} is redundant, not sure the effect it will have on ppl looking at the source. But I would ask myself why did they put that.
I think we should remove them and wait until/if/when we do the refactoring. We should not think ahead of ourselves.

I'm not sure that it really matters, we talked about deferring this problem to a second pull request in #606 (comment).

I see, but yeah normally you leave things clean because if you change your plan, and defer this refactoring as other priorities come first, then you will have this weird prop for some time in your documentation.

However, I don't think that developers will ever see this prop as the plan is to remove it before the next release.

Well we should release tomorrow or wednesday if we follow our plan...
Also I would argue that if we do showToolbar, we should consider doing showFooter...

showToolbar, we should consider doing showFooter.

I disagree, I think that https://material-ui.com/guides/api/#property-naming is more important for consistency. I also think that we don't need showToolbar nor hideToolbar.

Well we should release tomorrow or wednesday if we follow our plan...

I think that we should either delay the release of refactor the toolbar before merging this one.

DanailH added 4 commits November 18, 2020 15:22

Set up the Density Picker component, density prop and icons

9e22a14

Update grid state on density change

bd1f090

Get latest from upstream and fix lint errors

9e8f6cd

Adjust storybook example

e1de458

DanailH added the new feature New feature or request label Nov 19, 2020

DanailH requested review from dtassone and oliviertassinari November 19, 2020 15:11

DanailH self-assigned this Nov 19, 2020

oliviertassinari changed the title ~~Feature/data grid 567 density picker~~ [DataGrid] Add density picker Nov 19, 2020

oliviertassinari reviewed Nov 19, 2020

View reviewed changes

v4.0.0-alpha.10

f0b5a2d

dtassone reviewed Nov 23, 2020

View reviewed changes

packages/grid/_modules_/grid/components/DensityPicker.tsx Outdated Show resolved Hide resolved

DanailH added 5 commits November 24, 2020 11:14

Merge branch 'master' of github.com:mui-org/material-ui-x

0d38326

Rebase against latest from master

eeae76e

Reorganise the Density Picker logic

71cd4a4

Get latest from master

a56ce6e

Introduce density state to separate the provided props from the manag…

786335f

…ed state

DanailH marked this pull request as ready for review November 26, 2020 11:00

DanailH requested review from mbrookes, oliviertassinari and dtassone November 26, 2020 11:05

DanailH added 5 commits November 26, 2020 14:00

Remove custom transition for density picker

04a0247

Fix the unit test

59e358c

Trigger Build

0b294ff

Resolve conflicts

419233b

Trigger Build

3b35bfa

DanailH and others added 5 commits December 4, 2020 09:59

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

993bc97

Co-authored-by: Matt <github@nospam.33m.co>

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

12c353b

Co-authored-by: Matt <github@nospam.33m.co>

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

b5c8809

Co-authored-by: Matt <github@nospam.33m.co>

Fix menu possition

03b2d1b

Merge branch 'feature/DataGrid-567-density-picker' of github.com:Dana…

1d04a90

…ilH/material-ui-x into feature/DataGrid-567-density-picker

Fix doc formatting

a621d11

DanailH requested a review from mbrookes December 4, 2020 09:21

oliviertassinari reviewed Dec 4, 2020

View reviewed changes

mbrookes approved these changes Dec 6, 2020

View reviewed changes

Get latest from master and resolve conflicts

0ae6ff2

oliviertassinari reviewed Dec 7, 2020

View reviewed changes

packages/storybook/src/stories/grid-toolbar.stories.tsx Outdated Show resolved Hide resolved

oliviertassinari reviewed Dec 7, 2020

View reviewed changes

packages/storybook/src/stories/grid-toolbar.stories.tsx Outdated Show resolved Hide resolved

Remove knobs

6ef9806

dtassone reviewed Dec 7, 2020

View reviewed changes

DanailH merged commit d8b2997 into mui:master Dec 8, 2020

DanailH mentioned this pull request Dec 8, 2020

[DataGrid] Rename hideToolbar prop to showToolbar #706

Merged

[DataGrid] Add Density selector #606

[DataGrid] Add Density selector #606

Conversation

DanailH commented Nov 19, 2020 • edited by mbrookes

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

oliviertassinari Nov 19, 2020 • edited

Choose a reason for hiding this comment

mbrookes Nov 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrookes Nov 23, 2020 • edited

Choose a reason for hiding this comment

mbrookes Nov 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Nov 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 1, 2020 • edited

Choose a reason for hiding this comment

oliviertassinari Nov 19, 2020 • edited

Choose a reason for hiding this comment

dtassone commented Nov 23, 2020 • edited

DanailH commented Nov 23, 2020

dtassone commented Nov 23, 2020

DanailH commented Nov 26, 2020

oliviertassinari commented Dec 3, 2020 • edited

mbrookes commented Dec 3, 2020 • edited

oliviertassinari commented Dec 3, 2020

mbrookes commented Dec 4, 2020

oliviertassinari commented Dec 4, 2020

mbrookes commented Dec 4, 2020

DanailH commented Dec 4, 2020 • edited by oliviertassinari

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanailH commented Dec 4, 2020

oliviertassinari commented Dec 4, 2020 • edited

mbrookes commented Dec 4, 2020 • edited

DanailH commented Dec 6, 2020 • edited

oliviertassinari commented Dec 6, 2020 • edited

oliviertassinari commented Dec 6, 2020

DanailH commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Dec 7, 2020 • edited

Choose a reason for hiding this comment

DanailH commented Nov 19, 2020 •

edited by mbrookes

oliviertassinari left a comment •

edited

oliviertassinari Nov 19, 2020 •

edited

mbrookes Nov 23, 2020 •

edited

mbrookes Nov 23, 2020 •

edited

mbrookes Nov 24, 2020 •

edited

oliviertassinari Nov 25, 2020 •

edited

oliviertassinari Dec 1, 2020 •

edited

oliviertassinari Nov 19, 2020 •

edited

dtassone commented Nov 23, 2020 •

edited

oliviertassinari commented Dec 3, 2020 •

edited

mbrookes commented Dec 3, 2020 •

edited

DanailH commented Dec 4, 2020 •

edited by oliviertassinari

oliviertassinari left a comment •

edited

oliviertassinari commented Dec 4, 2020 •

edited

mbrookes commented Dec 4, 2020 •

edited

DanailH commented Dec 6, 2020 •

edited

oliviertassinari commented Dec 6, 2020 •

edited

oliviertassinari Dec 7, 2020 •

edited

oliviertassinari Dec 7, 2020 •

edited