[Button] Add startIcon / endIcon props #17600

mbrookes · 2019-09-28T23:39:40Z

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

I have gone with startIcon / endIcon for now.

<Button
  variant="contained"
  color="secondary"
  startIcon={<DeleteIcon fontSize="small" />}
>
  Delete
</Button>

mui-pr-bot · 2019-09-28T23:49:28Z

@material-ui/core: parsed: +0.16% , gzip: +0.18%

Details of bundle changes.

Comparing: e89abcd...7b67635

bundle	parsed diff	gzip diff	prev parsed	current parsed	prev gzip	current gzip
@material-ui/core	+0.16% 🔺	+0.18% 🔺	334,418	334,959	91,191	91,351
@material-ui/core/Paper	0.00%	0.00%	69,049	69,049	20,524	20,524
@material-ui/core/Paper.esm	0.00%	0.00%	62,377	62,377	19,276	19,276
@material-ui/core/Popper	0.00%	0.00%	28,405	28,405	10,179	10,179
@material-ui/core/Textarea	0.00%	0.00%	5,082	5,082	2,134	2,134
@material-ui/core/TrapFocus	0.00%	0.00%	3,766	3,766	1,597	1,597
@material-ui/core/styles/createMuiTheme	0.00%	0.00%	16,351	16,351	5,821	5,821
@material-ui/core/useMediaQuery	0.00%	0.00%	2,488	2,488	1,052	1,052
@material-ui/lab	0.00%	0.00%	143,406	143,406	43,818	43,818
@material-ui/styles	0.00%	0.00%	51,780	51,780	15,355	15,355
@material-ui/system	0.00%	0.00%	15,583	15,583	4,339	4,339
Button	+0.68% 🔺	+0.60% 🔺	79,148	79,689	24,122	24,267
Modal	0.00%	0.00%	14,542	14,542	5,113	5,113
Portal	0.00%	0.00%	2,907	2,907	1,316	1,316
Rating	0.00%	0.00%	70,239	70,239	21,941	21,941
Slider	0.00%	0.00%	75,380	75,380	23,272	23,272
colorManipulator	0.00%	0.00%	3,835	3,835	1,519	1,519
docs.landing	0.00%	0.00%	54,267	54,267	14,344	14,344
docs.main	+1.75% 🔺	+1.44% 🔺	588,954	599,235	188,297	191,014
packages/material-ui/build/umd/material-ui.production.min.js	+0.16% 🔺	+0.13% 🔺	305,373	305,862	87,836	87,953

Generated by 🚫 dangerJS against 7b67635

oliviertassinari · 2019-09-29T10:38:04Z

Should we add a demo with the circular progress?

mbrookes · 2019-09-29T13:35:10Z

Should we add a demo with the circular progress?

afcb6d7 ? 🤷‍♂

("Crossed in the post"? 😄)

joshwooding

I was trying to think how this would affect FABs but it seems okay in its current state.

docs/src/modules/components/AppFrame.js

oliviertassinari · 2019-09-29T16:42:07Z

docs/src/modules/components/AppFrame.js

            >
-              <LanguageIcon />
              <Hidden xsDown implementation="css">


This div creates an invalid DOM structure: https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-17600--material-ui.netlify.com%2F

We might want to trigger the small version for a larger screen width, it flex shrinks otherwise:

Hidden doesn't support the component prop. Do we go with the default js implementation or fix that separately?

oliviertassinari · 2019-09-29T16:43:10Z

docs/src/modules/components/AppFrame.js

@@ -229,16 +226,14 @@ function AppFrame(props) {
              onClick={handleLanguageIconClick}
              data-ga-event-category="AppBar"
              data-ga-event-action="language"
+              startIcon={<LanguageIcon fontSize="small" />}


I'm not sure about the change:

Before:

After:

Oops, I should have tried it! I'll partially revert.

docs/src/pages/components/buttons/IconLabelButtons.js

mbrookes · 2019-09-29T19:35:10Z

One other thought, should children be replaced by label, in order to follow https://material-ui.com/guides/api/? (If so I would propose we don't deprecate children, just change which one is documented.)

oliviertassinari · 2019-09-29T20:09:45Z

One other thought, should children be replaced by label, in order to follow https://material-ui.com/guides/api/? (If so I would propose we don't deprecate children, just change which one is documented.)

This would be a strong chance. I see a couple of options:

We change the rule. Is this rule useful? Maybe what's most important is to use the children prop as many time it's the most relevant one for composition as much as possible.
We rename the children prop, label.
We go for the children prop all in. We inspect the children prop to find elements we interested in. Past experience has taught us the limits of this method.
We go for the children prop all in. We use CSS child selector. With global class names, it doesn't require to import the related component. We have never used it yet. Seems simple but only support the components we define beforehand or with a generic class name.

mbrookes · 2019-09-29T20:27:31Z

I'm in favour of option 1.

Option 2, to change children to label as the documented approach deviates from expectations set by the native <button> element.

Option 3, agreed.

Option 4. This risks a never ending stream of issues to support edge cases not covered by the current implementation.

oliviertassinari · 2019-09-29T20:34:44Z

Option 1 and 4 seems the most popular alternatives in the other frameworks. I might like 1 better. @fzaninotto any preference?

fzaninotto · 2019-09-29T21:00:22Z

yes, I prefer option 1, too.

oliviertassinari · 2019-09-30T09:50:35Z

@mbrookes I have pushed a commit with the following changes:

Handle fullWidth prop. It seems that we need to render the icons inside the label if we want to have the icons display correctly when the button is full width:
Remove the circular progress, it's probably better to handle the concern in a "button loading state" effort.
Try to handle the language picker. This one is subjective, feel free to go in a different direction

oliviertassinari · 2019-10-01T17:05:18Z

Should we force a font-size on the provided icon?

diff --git a/packages/material-ui/src/Button/Button.js b/packages/material-ui/src/Button/Button.js
index 1f8763984..e4b4f8358 100644
--- a/packages/material-ui/src/Button/Button.js
+++ b/packages/material-ui/src/Button/Button.js
@@ -217,6 +217,24 @@ export const styles = theme => ({
     marginRight: -4,
     marginLeft: 8,
   },
+  /* Styles applied to the icon element if supplied and `size="small"`. */
+  iconSizeSmall: {
+    '& > *': {
+      fontSize: 18,
+    },
+  },
+  /* Styles applied to the icon element if supplied and `size="medium"`. */
+  iconSizeMedium: {
+    '& > *': {
+      fontSize: 20,
+    },
+  },
+  /* Styles applied to the icon element if supplied and `size="large"`. */
+  iconSizeLarge: {
+    '& > *': {
+      fontSize: 22,
+    },
+  },
 });

 const Button = React.forwardRef(function Button(props, ref) {
@@ -238,8 +256,16 @@ const Button = React.forwardRef(function Button(props, ref) {
     ...other
   } = props;

-  const startIcon = startIconProp && <span className={classes.startIcon}>{startIconProp}</span>;
-  const endIcon = endIconProp && <span className={classes.endIcon}>{endIconProp}</span>;
+  const startIcon = startIconProp && (
+    <span className={clsx(classes.startIcon, classes[`iconSize${capitalize(size)}`])}>
+      {startIconProp}
+    </span>
+  );
+  const endIcon = endIconProp && (
+    <span className={clsx(classes.endIcon, classes[`iconSize${capitalize(size)}`])}>
+      {endIconProp}
+    </span>
+  );

   return (
     <ButtonBase

so it can be used like this:

<Button startIcon={<DeleteIcon />} />

or stick with?

<Button startIcon={<DeleteIcon fontSize="small" />} />

mbrookes · 2019-10-01T17:08:37Z

Providing the correct icon size sounds like a good idea.

oliviertassinari · 2019-10-01T17:12:07Z

Ok, I'm preparing a new commit.

oliviertassinari · 2019-10-01T17:17:34Z

I'm not sure that I have used the best approach to apply this CSS, happy to review a better approach.

mbrookes

Two more...

docs/src/modules/components/MarkdownDocs.js

mbrookes · 2019-10-01T19:32:25Z

I don't have the permission to push

Odd, you already did:

oliviertassinari · 2019-10-02T06:17:15Z

🎉

Floriferous · 2019-10-07T14:04:17Z

It's not very clear whether we should start using these props versus children? Is the result the same at this time?

oliviertassinari · 2019-10-07T15:03:39Z

@Floriferous Use the prop if you want correct visual output out from the box. If you need to custom the position, you might not need it.

mbrookes added the component: button This is the name of the generic UI component, not the React module! label Sep 28, 2019

oliviertassinari added the new feature New feature or request label Sep 29, 2019

mbrookes force-pushed the button-icons branch from f183479 to d86e41c Compare September 29, 2019 11:11

mbrookes force-pushed the button-icons branch from 45938ff to b6ec03f Compare September 29, 2019 14:04

mbrookes marked this pull request as ready for review September 29, 2019 14:12

joshwooding approved these changes Sep 29, 2019

View reviewed changes

oliviertassinari reviewed Sep 29, 2019

View reviewed changes

docs/src/pages/components/buttons/IconLabelButtons.js Outdated Show resolved Hide resolved

oliviertassinari force-pushed the button-icons branch from 8f2dc7a to 946490f Compare September 30, 2019 09:51

oliviertassinari approved these changes Sep 30, 2019

View reviewed changes

oliviertassinari added the PR: ready to ship label Sep 30, 2019

mbrookes and others added 10 commits October 1, 2019 18:49

[Button] Add start / end icon props

a8f480f

Add the loading button example

aaea320

Update docs internal usage

f87b8c3

Ninja edit

4aacc50

Add examples to spec, revise types

df549e9

Add tests

1363193

Prettier

e7c3bbc

Fix test

ee957c5

small changes

bece32a

handle fullWidth prop

d9160b4

oliviertassinari removed the PR: ready to ship label Oct 1, 2019

apply font size on the icon

eab3331

oliviertassinari force-pushed the button-icons branch from 946490f to eab3331 Compare October 1, 2019 17:17

oliviertassinari added the PR: needs review label Oct 1, 2019

mbrookes commented Oct 1, 2019

View reviewed changes

docs/src/modules/components/MarkdownDocs.js Outdated Show resolved Hide resolved

docs/src/modules/components/MarkdownDocs.js Outdated Show resolved Hide resolved

Apply suggestions from code review

7b67635

oliviertassinari added PR: ready to ship and removed PR: needs review labels Oct 1, 2019

oliviertassinari merged commit c7d1f1d into mui:master Oct 2, 2019

mbrookes deleted the button-icons branch October 2, 2019 08:24

oliviertassinari mentioned this pull request Nov 30, 2019

Where is button with icons and labels in the v1 beta? #8891

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button] Add startIcon / endIcon props #17600

[Button] Add startIcon / endIcon props #17600

mbrookes commented Sep 28, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Sep 28, 2019 •

edited

Loading

oliviertassinari commented Sep 29, 2019 •

edited

Loading

mbrookes commented Sep 29, 2019 •

edited

Loading

joshwooding left a comment •

edited

Loading

oliviertassinari Sep 29, 2019

oliviertassinari Sep 29, 2019

mbrookes Sep 29, 2019

oliviertassinari Sep 29, 2019

mbrookes Sep 29, 2019

mbrookes commented Sep 29, 2019 •

edited

Loading

oliviertassinari commented Sep 29, 2019 •

edited

Loading

mbrookes commented Sep 29, 2019

oliviertassinari commented Sep 29, 2019

fzaninotto commented Sep 29, 2019

oliviertassinari commented Sep 30, 2019

oliviertassinari commented Oct 1, 2019

mbrookes commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019

mbrookes left a comment

mbrookes commented Oct 1, 2019

oliviertassinari commented Oct 2, 2019

Floriferous commented Oct 7, 2019

oliviertassinari commented Oct 7, 2019

[Button] Add startIcon / endIcon props #17600

[Button] Add startIcon / endIcon props #17600

Conversation

mbrookes commented Sep 28, 2019 • edited by oliviertassinari Loading

mui-pr-bot commented Sep 28, 2019 • edited Loading

oliviertassinari commented Sep 29, 2019 • edited Loading

mbrookes commented Sep 29, 2019 • edited Loading

joshwooding left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari Sep 29, 2019

Choose a reason for hiding this comment

oliviertassinari Sep 29, 2019

Choose a reason for hiding this comment

mbrookes Sep 29, 2019

Choose a reason for hiding this comment

oliviertassinari Sep 29, 2019

Choose a reason for hiding this comment

mbrookes Sep 29, 2019

Choose a reason for hiding this comment

mbrookes commented Sep 29, 2019 • edited Loading

oliviertassinari commented Sep 29, 2019 • edited Loading

mbrookes commented Sep 29, 2019

oliviertassinari commented Sep 29, 2019

fzaninotto commented Sep 29, 2019

oliviertassinari commented Sep 30, 2019

oliviertassinari commented Oct 1, 2019

mbrookes commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019

oliviertassinari commented Oct 1, 2019

mbrookes left a comment

Choose a reason for hiding this comment

mbrookes commented Oct 1, 2019

oliviertassinari commented Oct 2, 2019

Floriferous commented Oct 7, 2019

oliviertassinari commented Oct 7, 2019

mbrookes commented Sep 28, 2019 •

edited by oliviertassinari

Loading

mui-pr-bot commented Sep 28, 2019 •

edited

Loading

oliviertassinari commented Sep 29, 2019 •

edited

Loading

mbrookes commented Sep 29, 2019 •

edited

Loading

joshwooding left a comment •

edited

Loading

mbrookes commented Sep 29, 2019 •

edited

Loading

oliviertassinari commented Sep 29, 2019 •

edited

Loading