[actions] Rename disableActionSpacing to disableSpacing

[oliviertassinari]

Uses the same core solution for ExpansionPanelActions, CardActions and DialogActions.

Breaking changes

• [CardActions] Rename the disableActionSpacing prop disableSpacing.
• [CardActions] Remove the disableActionSpacing CSS class.
• [CardActions] Rename the action CSS class spacing.
• [DialogActions] Rename the disableActionSpacing prop disableSpacing.
• [DialogActions] Rename the action CSS class spacing.
• [ExpansionPanelActions] Rename the action CSS class spacing.

This change in the bundle size is interesting:

@material-ui/core/Textarea | -9.19% | -7.30% | 5,866 | 5,327 | 2,465 | 2,285

[mui-pr-bot]

@material-ui/core: parsed: -0.11% 😍, gzip: -0.12% 😍

Details of bundle changes.

Comparing: 4e4a2ed...2cf169d

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	-0.11%	-0.12%	315,304	314,970	85,279	85,177
@material-ui/core/Paper	0.00%	-0.01%	67,277	67,277	19,975	19,974
@material-ui/core/Paper.esm	0.00%	0.00%	60,639	60,639	18,883	18,883
@material-ui/core/Popper	-1.54%	-1.35%	34,906	34,370	11,861	11,701
@material-ui/core/Textarea	-9.19%	-7.30%	5,866	5,327	2,465	2,285
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	15,898	15,898	5,773	5,773
@material-ui/core/useMediaQuery	0.00%	0.00%	2,106	2,106	974	974
@material-ui/lab	-0.15%	-0.13%	144,182	143,959	43,530	43,475
@material-ui/styles	0.00%	-0.01%	50,831	50,831	15,020	15,018
@material-ui/system	0.00%	0.00%	11,765	11,765	3,929	3,929
Button	-0.25%	-0.17%	88,247	88,024	26,501	26,457
Modal	-0.27%	-0.19%	82,524	82,301	24,821	24,773
colorManipulator	0.00%	0.00%	3,904	3,904	1,543	1,543
docs.landing	0.00%	0.00%	50,908	50,908	11,210	11,210
docs.main	-0.04%	-0.03%	647,004	646,730	201,944	201,875
packages/material-ui/build/umd/material-ui.production.min.js	-0.09%	-0.06%	291,938	291,681	82,288	82,236

Generated by 🚫 dangerJS against 2cf169d

[synchronos-t]

That "[CardActions] Rename the action CSS class spacing" is a tad underwhelming expression, as the action class was actually removed. The replacing spacing class applies to a completely different element, the root, and not the children.

(I used to override the children of MuiCardActions in the theme overrides, as it was an easy way to override all the buttons inside CardActions, but now it doesn't work any more. *sigh* Well, I'll go now and override them in all the contexts instead or create a themed button for it.)

[oliviertassinari]

@synchronos-t Have you tried to do something like this?

.MuiCardActions-root > * {
  background-color: red;
}

[NMinhNguyen]

@oliviertassinari I was actually gonna use cloneChildrenWithClassName (or copy & paste) in my project, but then saw that it got removed in this PR. Is it because it uses cloneElement and there are issues with it (#12921)?

[oliviertassinari]

@NMinhNguyen cloneElement behavior is hidden to the component users. It's unpredictibale. Also, it makes style overrides more likely not to work. People often forget to forward the props.

[NMinhNguyen]

it makes style overrides more likely not to work

@oliviertassinari Do you mean because people often forget to forward the props? Or for another reason? Makes sense overall though. Do you think that context-based class propagation would be better than cloneElement or would you try to use child selectors in the parent instead?

[oliviertassinari]

Yes, people have to know they need to forward the props, they forget it. Using the context has an overhead, it's better to rely on CSS where possible.

[eps1lon]

Using the context has an overhead

I'm not aware of that. What's the overhead compared to cloneElement?

[oliviertassinari]

@eps1lon It has an overhead vs using CSS. The pros of CSS for this case is that it targets any possible DOM node (less room for mistakes), it's fast to run (no JavaScript) and it's fast to write (you don't need to export a module or touch multiple files).

[NMinhNguyen]

@eps1lon @oliviertassinari I actually remembered a nasty caveat with a context-based solution. At work, we also had a use case where a Parent component wanted to influence how a Child is styled. Now, since both the Parent and Child were defining a style using a single className, then specificity-wise it came down to source order, i.e. which component in JS is executed first (essentially whose withStyles HOC runs first):

import Parent from './Parent';
import Child from './Child';

// or 

import Child from './Child';
import Parent from './Parent';

This was annoying, and the only way to make the order more deterministic was for the Parent to import the child:

// Parent.js
// Side-effecting import - the Parent doesn't render the Child :(
import './Child';

This approach seemed somewhat brittle and kinda relied on the fact that bundlers don't try to tree shake the unused Child import

[oliviertassinari]

@NMinhNguyen That's an issue with cloneElement right? (not context)

[NMinhNguyen]

@oliviertassinari the issue I described above was when we used Context :) We had the Child component render a static class e.g.

function Child() {
  return <span className="static-class" />
}

const parentStyles = {
  root: {
    '&.static-class': {} // higher specificity because of root container + descendant selector
  }
}

And we wanted to remove this dependency on static classes.. So we introduced context to implicitly pass down childClassName provided by Parent. Because Child wasn't a direct dependency of Parent, then it depended on the order in which our consumers imported them. Does that make sense?

[josgraha]

nice css skills @oliviertassinari