[Button] Icon Button Spacing Uneven

[una]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

"A" spacing does not equal "B" spacing

Expected Behavior 🤔

Solution: Add margin-right: -4px; to icons in buttons with icon following button (adjusting "B" spacing)

Steps to Reproduce 🕹

Steps:

1. Create button with trailing icon
2. Witness button

Context 🔦

Material.io spec compliance
See also: https://material-components.github.io/material-components-web-catalog/#/component/button?type=raised

Your Environment 🌎

Tech	Version
Material-UI	latest @4

[mbrookes]

The button has 16px of padding to the left of the text portion of the label, and to the right of the icon, which is correct per the spec.

I had wondered recently though whether we should support startIcon and endIcon props, and remove the need for manually applying these styles.

Edit: I should have double-checked the spec before commenting! The current spec now shows a button with icon (it didn't historically), and has 12px between the icon and button edge, for an 18px icon.

Two more reasons we should support this usage with props...

[oliviertassinari]

@una Thank you for looking at our components!

Two more reasons we should support this with usage with props...

@mbrookes I agree, @fzaninotto has been advocating for it for a year now. I had this use case at a few occasions in the past, it was frustrating. I would propose diff close to this one:

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 4e1304ff1..ad9a0f786 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -179,6 +179,16 @@ export const styles = theme => ({
   fullWidth: {
     width: '100%',
   },
+  startAdornment: {
+    display: 'inherit',
+    marginRight: 8,
+    marginLeft: -4,
+  },
+  endAdornment: {
+    display: 'inherit',
+    marginRight: -4,
+    marginLeft: 8,
+  },
 });

 const Button = React.forwardRef(function Button(props, ref) {
@@ -190,9 +200,11 @@ const Button = React.forwardRef(function Button(props, ref) {
     component = 'button',
     disabled = false,
     disableFocusRipple = false,
+    endAdornment: endAdornmentProp,
     focusVisibleClassName,
     fullWidth = false,
     size = 'medium',
+    startAdornment: startAdornmentProp,
     type = 'button',
     variant = 'text',
     ...other
@@ -223,6 +235,13 @@ const Button = React.forwardRef(function Button(props, ref) {
     classNameProp,
   );

+  const startAdornment = startAdornmentProp && (
+    <span className={classes.startAdornment}>{startAdornmentProp}</span>
+  );
+  const endAdornment = endAdornmentProp && (
+    <span className={classes.endAdornment}>{endAdornmentProp}</span>
+  );
+
   return (
     <ButtonBase
       className={className}
@@ -234,7 +253,11 @@ const Button = React.forwardRef(function Button(props, ref) {
       type={type}
       {...other}
     >
-      <span className={classes.label}>{children}</span>
+      {startAdornment}
+      <span className={classes.label}>
+        {children}
+      </span>
+      {endAdornment}
     </ButtonBase>
   );
 });

Then it can be used like this:

import React from 'react';
import Button from '@material-ui/core/Button';
import CircularProgress from '@material-ui/core/CircularProgress';

export default function App() {
  return (
    <Button
      variant="contained"
      color="primary"
      startAdornment={<CircularProgress color="inherit" size={20} />}
      loading
    >
      Loading
    </Button>
  );
}

(the loading prop would be for another pull-request)

or

import React from 'react';
import Button from '@material-ui/core/Button';
import DeleteIcon from '@material-ui/icons/Delete';

export default function App() {
  return (
    <Button
      variant="contained"
      color="secondary"
      endAdornment={<DeleteIcon fontSize="small" />}
    >
      Delete
    </Button>
  );
}

Any concern left?

[una]

Props sound like a good idea here to adjust this style based on position. Thanks for looking into it!

[fzaninotto]

Eternal love to you if you implement icon support in material-ui 😍

[mbrookes]

Any concern left?

The word "adornment? 😄 (I never really liked it for TextField, but I guess we're stuck with it now.)

[oliviertassinari]

@mbrookes We could propose a change with v5. Do you have a better name in mind? It would need to be significantly better to justify a breaking change 😁.

[mbrookes]

Do you have a better name in mind?

I was afraid you might ask that! 😆 Not specifically, and no, not worth a breaking change just to appease my personal sensibilities!

[mbrookes]

@fzaninotto And to you if you would like to submit the PR. (Okay, perhaps not eternal... 😄)