[IconButton] Implement a new edge prop

[jedwards1211]

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

One of the most common things we all have to do with IconButtons is apply marginLeft: -12 or
marginRight: -12 to counteract the padding and fix the icon's horizontal alignment with content above or below. (It's common in your demos too)

It gets a bit tedious to fix every case with JSS styles. Let's make it easier for ourselves! ✨

With this PR, <IconButton align="left"> will apply marginLeft: -12 and align="right" will apply marginRight: -12. For size="small", it will use -3 for the margin instead.

[mui-pr-bot]

Details of bundle changes.

Comparing: 099bc5e...c81bf03

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.05% 🔺	+0.07% 🔺	362,378	362,562	92,048	92,116
@material-ui/core/Paper	0.00%	0.00%	68,773	68,773	20,155	20,155
@material-ui/core/Paper.esm	0.00%	0.00%	63,294	63,294	19,274	19,274
@material-ui/core/Popper	0.00%	0.00%	30,458	30,458	10,557	10,557
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	17,284	17,284	5,697	5,697
@material-ui/core/useMediaQuery	0.00%	0.00%	2,486	2,486	1,049	1,049
@material-ui/lab	0.00%	0.00%	170,613	170,613	50,038	50,038
@material-ui/styles	0.00%	0.00%	57,118	57,118	16,253	16,253
@material-ui/system	0.00%	0.00%	17,041	17,041	4,498	4,498
Button	0.00%	0.00%	91,106	91,106	26,915	26,915
Modal	0.00%	0.00%	90,214	90,214	26,715	26,715
colorManipulator	0.00%	0.00%	3,232	3,232	1,298	1,298
docs.landing	0.00%	0.00%	51,908	51,908	11,302	11,302
docs.main	+0.04% 🔺	+0.04% 🔺	646,179	646,417	200,782	200,854
packages/material-ui/build/umd/material-ui.production.min.js	+0.06% 🔺	+0.08% 🔺	310,946	311,128	85,146	85,217

Generated by 🚫 dangerJS against c81bf03

[eps1lon]

Could you apply the new prop to every search result you posted? The description implies it's common with multiple search result but the solution is only applied to a single instance.

[oliviertassinari]

@jedwards1211 Why do you want to make the IconButton own the logic responsible for a correct layout within a Toolbar? Shouldn't it be the other way around?

[eps1lon]

@jedwards1211 Why do you want to make the IconButton own the logic responsible for a correct layout within a Toolbar? Shouldn't it be the other way around?

Well because the IconButton itself affects layout. If it didn't have any margin I would agree. However by moving the logic inside the IconButton we can safely adjust the margin without affect layout because layout isn't hardcoded but done semantically.

[oliviertassinari]

Well because the IconButton itself affects layout.

@eps1lon I see, thanks for clarifying it. What do you think of a single boolean flag prop that that also works with the size property? We could name it antiPadding or something else. Basically, it would counter the padding needed for the ripple effect in all the directions.

.anti-padding {
  margin: -12px;
}

.anti-padding-small {
  margin: -3px
}

I think that this strategy can cover more use case, it sounds great to me.

[jedwards1211]

I'm not sure negative margin in all directions would work flawlessly in all cases. Maybe... I'll give it a try, it may mess up the vertical alignment though.

[jedwards1211]

@eps1lon yes definitely, I'll update the other places in the demos, I just wanted to wait to do that work till I knew this would be accepted, we've agreed on the API

[jedwards1211]

@oliviertassinari okay, unfortunately margin: -12 doesn't quite work. I applied margin: -12 to the account button in the PrimarySearchAppBar demo and you can see that it messes up the spacing relative to the other icons and the border shape:

I think it's best to stick with marginLeft: -12 or marginRight: -12. I think it's actually rare that anyone would need -12 margin on both sides. I'm happy to rename the prop to antiPadding="left" if you want though.

[joshwooding]

antiPadding for me does not clearly convey what the property is supposed to do

[jedwards1211]

@joshwooding I agree; I think align is a bit awkward too (because it sounds like CSS justification) but align="left" does sort convey the purpose that the left side of the icon needs to be aligned with something else

[oliviertassinari]

@jedwards1211 Ok, my bad, poor proposition. Instead, could we rely on CSS first & last child selectors?

[jedwards1211]

@oliviertassinari I think if that were a flexible enough approach it would have already been done by whomever added the 12px padding to IconButton in the first place. There is marginLeft: -12 and marginRight: -12 applied to IconButtons on a case-by-case basis throughout your existing demos...I'm just codifying your existing approach into an API.
If you imagine

<AppBar>
  My App
  <AlertIconButtons />
  <AccountIconButtons />
</AppBar>

Where AccountIconButtons and AlertIconButtons each render several IconButton inside of a <div>, the first & last child CSS would be triggered on some of those IconButtons in an undesirable place. I really think this prop is the most convenient overall because it gives people full control without costing many keystrokes.

[oliviertassinari]

I think if that were a flexible enough approach it would have already been done by whomever added the 12px padding to IconButton in the first place.

@jedwards1211 If I had to guess, I would say that I'm the one who added the negative margin. We have never spend any effort into improving this implement. Maybe we can find something better given the field has been little explored yet. But no, I can't think of anything better, somebody else?

I like the property proposition. Regarding the naming. What do you think of?

edge?: 'start' | 'end' | false;

[jedwards1211]

@oliviertassinari did you mean to post your last message on my other PR? Anyway thanks!
edge is a smart name for the prop, I like it!

So are you saying you want me to put the padding fixes into this PR?

[jedwards1211]

As far as using the ListItem directly, unfortunately it breaks encapsulation in some cases (the list is in a separate component that extra stuff like the inputs can't or shouldn't be passed into)

[eps1lon]

• We can close Document theme.mixins.gutters() #11539 by removing theme.mixins.gutters(). The only valid use case I can see in the codebase is in the Toolbar component.

We should be careful with that given

[...] the fact that thousands (44k on a rough search albeit most should no longer be maintained) of repositories are using the same mixin [...]
-- #11539 (comment)

[oliviertassinari]

edge is a smart name for the prop, I like it!

@eps1lon Is right, better move carefully. We can split the effort into different pull requests. I think that the first order of business is to abstract the negative margin styles.

[jedwards1211]

Okay I renamed the prop to edge and searched the docs for marginLeft: - and marginRight: - to try to find all the places I can use this new prop instead. If anyone finds additional places I should use it let me know!

[oliviertassinari]

What do you think of start | end? It's an open question, I'm not sure what's best.

On our codebase

• We are using the following values for the InputAdornment component:

position: PropTypes.oneOf(['start', 'end']),

• We are using the following values for the FormControlLabel component:

labelPlacement: PropTypes.oneOf(['end', 'start', 'top', 'bottom']),

• We are using the following values for the Tooltip component:

  placement: PropTypes.oneOf([
    'bottom-end',
    'bottom-start',
    'bottom',
    'left-end',
    'left-start',
    'left',
    'right-end',
    'right-start',
    'right',
    'top-end',
    'top-start',
    'top',
  ]),

• Everywhere else we use left | right.

Benchmarking

CSS specification

• The align-items CSS property uses the following values:

align-items?: 'flex-start' | 'flex-end' | 'center' | 'baseline'

I'm wondering if the CSS property values were influenced by RTL considerations or if it's simply related to the flew direction that can be either the x or y axis. Ok, it's probably not a good example, let me look deeper.

• The CSS Grid model seems to have replace the left right concepts with start end: https://css-tricks.com/snippets/css/complete-guide-grid/.

Misc

• https://medium.com/@elad/new-css-logical-properties-bc6945311ce7

cc @mui-org/core-contributors

[jedwards1211]

Huh, does Toolbar automatically reverse its content in RTL mode? If so then maybe you're right that we should reverse the margin applied by edge in RTL mode.

[oliviertassinari]

Does Toolbar automatically reverse its content in RTL mode?

Yes, it does, you can try it on https://next.material-ui.com/. I'm trying to understand what would be more idiomatic to our users. I believe very few of them care about RTL. Hence my question, it's not obvious to me.

[jedwards1211]

Okay, I will change the enum to start | end then. I think it is probably most convenient if this prop does the same thing conceptually in RTL mode?

[jedwards1211]

Seems to me there's enough precedent for users understanding start | end, even though it's a bit less obvious than left | right for InputAdornment and so forth

[jedwards1211]

@oliviertassinari oh, so looks like a JSS plugin is used to automatically flip all left and right properties in RTL mode and I don't need to check for RTL mode in my code changes -- am I understanding correctly?

[oliviertassinari]

yes

[kgregory]

start | end 👍

[jedwards1211]

A related issue: #14784.

updated

[kgregory]

This is great.

[jedwards1211]

@KenGregory please help me advocate for adding an edge prop to Button too :)...I haven't managed to convince Olivier yet in #14784

[oliviertassinari]

@jedwards1211 Thanks

[jedwards1211]

Thank you guys!