[Table] Add dense support

[leMaik]

Breaking change

• We have removed the deprecated numeric property.

-<TableCell numeric>{row.calories}</TableCell>
+<TableCell align="right">{row.calories}</TableCell>

• We have removed the fixed height property on the table row. The cell height is computed by the browser using the padding and line-height.
• The dense mode was promoted to a different property:

-<TableCell padding="dense" />
+<TableCell size="small" />

---

Currently, setting padding="dense" doesn't really do much for the Table. As described in #14521, the Material Design specs now contain some information about density. This is useful for tables in desktop web apps, where finger-friendlyness isn't important.

I also experimented with adding a new dense prop and passing it through via TableContext, but that adds a lot more code for the same benefit.

As it is now, this would be a breaking change because the old padding="dense" behavior is removed. I don't know what the old behavior was used for. 🤷‍♂️

Also, dense pagination is weird… the IconButtons aren't really made for smaller sizes. When using a CheckBox in the table, the padding of it must be set¹ to 0, then it'll just work and look as in the guidelines:

¹ Maybe we could add padding="none" for IconButtons?

• I have followed (at least) the PR section of the contributing guide.

[mbrookes]

House style...

[leMaik]

@mbrookes A linter rule for the house style, e.g. not importing non-default React exports, would be nice.

Sorry for not applying your suggestions with GitHub, I didn't know you can batch them and didn't want to have one commit for every line.

[leMaik]

Any other feedback, @mbrookes or @eps1lon? :)
All that I'd still want to do is update the table demo; should I add a new, dense, table (with checkboxes) or should I add a toggle to make the existing table dense?

[eps1lon]

[...] or should I add a toggle to make the existing table dense?

Sounds like the better option to me.

[mbrookes]

Here's how the uses of 'dense' compare:

Component	prop	type	values	style applied
FilledInput	margin	enum	'dense', 'none'	paddingTop, paddingBottom
FormControl	margin	enum	'none', 'dense', 'normal'	marginTop, marginBottom
FormHelperText	margin	enum	'dense', undefined	marginTop
InputBase	margin	enum	'dense', 'none'	{}
InputLabel	margin	enum	'dense', undefined	transform
ListItem	dense	bool	true, false	paddingTop, paddingBottom
OutlinedInput	margin	enum	'dense', 'none'	paddingTop, paddingBottom
TableCell	padding	enum	'default', 'checkbox', 'dense', 'none'	padding / paddingRight
TextField	margin	enum	'none', 'dense', 'normal'	(passed down)
ToolBar	variant	enum	'regular', 'dense'	minHeight

Fab API says: "small is equivalent to the dense button styling.", but Button doesn't have dense any more.

[leMaik]

@mbrookes Hm... I was thinking about adding a dense prop to the table, but that would have required more changes and more code. Also, it affects the padding of the cells, so I'd say the name is fine.

ListItem is odd, how about changing the API to padding="normal|dense"? Then again, why introduce another breaking change?

[mbrookes]

I was just pointing out the inconsistencies in our current API regarding prop name, type and values.

why introduce another breaking change

To make the API more consistent (where it makes sense).

it affects the padding of the cells, so I'd say the name is fine.

I don't think the name should necessarily reflect the CSS styles affected. We don't for most other style props. For example, as it stands, most of the props called margin set padding, not margin...

I'm not suggesting changing it in this PR per se, but I do think we need to revisit the subject for the components listed in the table above.

[leMaik]

@mbrookes So… can we proceed with this PR? I'm done with the implementation, there hasn't been any feedback for some days. Reviews/approvals would speed this up a bit. 😅

[mbrookes]

@leMaik

What's the purpose ofpadding="none"? It just seems to makes the table look broken.

Do we need two demos? I'd prefer we keep the basic demo as simple as possible.

The demos use hooks, but aren't named as such. I guess that's okay though if we're getting rid of hooks as a specific category for v4, and only having js (with hooks), or ts.

[leMaik]

@mbrookes I didn't add padding="none". I have no idea what it's there for, I didn't add it. I just included in in the padding selector because it's a valid value.

The demo uses hooks to reduce code noise (would need to be a class component with state otherwise). I adjusted both demos because I wanted to show how to adjust the checkboxes to fit in dense tables.

[mbrookes]

I have no idea what it's there for

Looking at the PR #12415, I'd guess it was added simply because TableCell has it. Not sure how much use it is on the entire table - more likely to be used on a single column; but I guess it's harmless. I would remove it from the select though.

However padding="checkbox" definitely has no business being available in Table. You could remove that.

The demo uses hooks to reduce code noise

Nothing wrong with using hooks. My point was that the file names are wrong, but that it doesn't matter so much, as we're planning on getting rid of the -hooks.js extension anyway. It just means the wrong icon is highlighted for now.

I adjusted both demos because I wanted to show how to adjust the checkboxes to fit in dense tables.

Which it does. My question was do we also need the select in the basic demo? Like I said, I'd prefer we keep the basic demo as simple as possible. That said, it's much harder to unpick what's needed to make a table dense from the 'Sorting and Selecting' demo...

While looking at that again, I noticed that in our current implementation, and the new dense one, the position of the checkbox WRT the left edge is incorrect:

In both cases it should be 16px with a 24px checkbox:

From the vertical grid lines (which we lack), it seems the checkbox is in the same column as the text (the text is a label on the checkbox). That may be for another PR though!

[leMaik]

That said, it's much harder to unpick what's needed to make a table dense from the 'Sorting and Selecting' demo...

@mbrookes That's what I thought, too and why I added it to both demos. 🤔 Adding a new demo for the dense table is an overkill too, on the other hand.

That may be for another PR though!

Nice! That'll be my contribution for next week. 😎

[mbrookes]

@leMaik Sorry, thought I'd left a comment, but must not have submitted it. It looks like the standard and dense Checkboxes now have the same padding as each other, but not as the v2 spec (16px). Had you planned to fix that?

[leMaik]

@mbrookes Oh, the padding changed. I thought it was only the dense checkbox being inconsistent. I'll fix it.

Edit: I have changed it locally, but there are more changes to the data table specs (e.g. the sorting arrow is now 18px instead of 16px wide). Let's put these changes into a new PR.

Edit²: The EnhancedTable demo is missing Pie. 🍰 🤓

[mbrookes]

Pie

Lol

Yeah. Let's not overload one PR.

[eps1lon]

Maybe wrap the displayed demos in their own themeprovider and give people the ability to play around with it. Gives us another way of teaching how to override stuff (props, style etc) and in this case the ability to see how dense tables look for every demo.

[leMaik]

I don't see a reason not to include the default vs. normal change in this already breaking change.

[oliviertassinari]

The enhanced hook and non hook demos are different. How should we unify them?

[oliviertassinari]

I have tried to list the breaking changes in the pull request description. Have I missed one?

[leMaik]

@oliviertassinari "dense" padding was removed for the Table and TableCell and replaced with size="small", that's also a breaking change

[oliviertassinari]

@leMaik Oh right, thanks. I have added this breaking change in the pull request description. I'm rebasing it :).

[oliviertassinari]

We had 6 conflits after merging #14536. I went with the simplest resolution strategy: squash + resolve. I'm sorry, we have lost the commit history.

[oliviertassinari]

@leMaik well done

[ljvanschie]

The TableCell class keys have not been updated yet. For example, it still contains paddingDense which does not exist anymore.

Also, numeric is still a prop.

[oliviertassinari]

@ljvanschie Well spotted! Do you want to submit a pull request that fixes the TypeScript class key definition? :)