[Button] Remove label span #26666

siriwatknp · 2021-06-09T07:53:36Z

BREAKING CHANGES

The span element that wraps children has been removed. label classKey is also removed.

Preview
https://deploy-preview-26666--material-ui.netlify.app/components/buttons/

Changes Summary

remove label classes and element from Button and LoadingButton
fix LoadingButton position: center styling because it rely on label before.

History

button > span > children exists as a workaround for flex container bug on button BUT not anymore, so this PR remove the span.

✅ Chrome 83+ (latest 90)
✅ Safari 11+ (latest 14)
✅ Edge
✅ Firefox 63 (latest 89)

https://github.com/philipwalton/flexbugs#flexbug-9

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

siriwatknp · 2021-06-09T07:54:58Z

packages/material-ui-lab/src/LoadingButton/LoadingButton.js

+  transition: theme.transitions.create(['background-color', 'box-shadow', 'border-color'], {
+    duration: theme.transitions.duration.short,
+  }),


add transition (exclude color) due to this glitch.

loading-button-transition.mp4

It still looks off:

The non centered cases are worse.

mui-pr-bot · 2021-06-09T07:55:50Z

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 6aabb2a

siriwatknp · 2021-06-09T07:57:09Z

packages/material-ui-lab/src/LoadingButton/LoadingButton.js

+  ...(styleProps.loadingPosition === 'center' && {
+    [`&.${buttonClasses.disabled}`]: {
+      color: 'transparent',
+    },
+  }),
+}));


added to fix this.

siriwatknp · 2021-06-09T07:58:09Z

packages/material-ui-lab/src/LoadingButton/LoadingButton.js

@@ -88,6 +89,7 @@ const LoadingButtonLoadingIndicator = styled('div', {
  ...(styleProps.loadingPosition === 'center' && {
    left: '50%',
    transform: 'translate(-50%)',
+    color: theme.palette.action.disabled,


because of the above fix. color: transparent makes indicator disappear, so need to force the color same is disabled state in Button

packages/material-ui/src/Button/Button.test.js

mnajdova

I could not spot any issues, we should just fix the test before merging

mnajdova

Looks good, I could not test only Safari.

oliviertassinari · 2021-06-09T22:43:00Z

I believe that merging these changes means closing #26412. AFAIK, the solution was depending on the span, cc @eps1lon to confirm or not.

oliviertassinari

Regarding the transition when centered, the only solution I could find is adding the span back for the loading label. So it can also support more complex children

I could find one change to improve the transition for the start and end icons that works both on this PR and HEAD:

diff --git a/packages/material-ui-lab/src/LoadingButton/LoadingButton.js b/packages/material-ui-lab/src/LoadingButton/LoadingButton.js
index 03195089be..c4cc467b73 100644
--- a/packages/material-ui-lab/src/LoadingButton/LoadingButton.js
+++ b/packages/material-ui-lab/src/LoadingButton/LoadingButton.js
@@ -56,11 +56,11 @@ const LoadingButtonRoot = styled(Button, {
   transition: theme.transitions.create(['background-color', 'box-shadow', 'border-color'], {
     duration: theme.transitions.duration.short,
   }),
-  [`& .${loadingButtonClasses.startIconLoadingStart}`]: {
-    visibility: 'hidden',
-  },
-  [`& .${loadingButtonClasses.endIconLoadingEnd}`]: {
-    visibility: 'hidden',
+  [`& .${loadingButtonClasses.startIconLoadingStart}, & .${loadingButtonClasses.endIconLoadingEnd}`]: {
+    transition: theme.transitions.create(['opacity'], {
+      duration: theme.transitions.duration.short,
+    }),
+    opacity: 0,
   },
   ...(styleProps.loadingPosition === 'center' && {
     [`&.${buttonClasses.disabled}`]: {

What it does on HEAD:

oliviertassinari · 2021-06-09T23:03:48Z

packages/material-ui-lab/src/LoadingButton/LoadingButton.js

+  transition: theme.transitions.create(['background-color', 'box-shadow', 'border-color'], {
+    duration: theme.transitions.duration.short,
+  }),


It still looks off:

The non centered cases are worse.

siriwatknp · 2021-06-10T02:52:24Z

Regarding the transition when centered, the only solution I could find is adding the span back for the loading label. So it can also support more complex children

In my opinion, adding the span back is a no go as we learned from other components that the structure should not surprise people.

Screen.Recording.2564-06-10.at.09.49.28.mov

Here, I think it is better. remove color from transition only for loadingPosition: 'center'. This way it does not affect other position.

oliviertassinari · 2021-06-10T21:35:46Z

There are two issues with the current animation: 1. the icons disappear instantly (which is quite visible), 2. the border-radius color isn't in sync with the text color. It doesn't feel as smooth as on #26666 (review). The last one can be fixed if we add a <span>.

Another issue of not using a span: this do no longer work:

      <LoadingButton
        onClick={handleClick}
        loading={loading}
        loadingIndicator="Loading..."
        variant="outlined"
      >
        Notifications <Badge>9</Badge>
      </LoadingButton>

PR: https://codesandbox.io/s/loadingbuttonstransition-material-demo-forked-0116b?file=/demo.js:1232-1443

HEAD: https://codesandbox.io/s/zch00?file=/demo.tsx

is a no go as we learned from other components that the structure should not surprise people

Agree with this downside. It's definitely a tradeoff.

oliviertassinari

I have added a new commit. Happy to moving forward as is. At least, we are conscious about the <span> tradeoff:

Cons:

Surprising DOM structure for new users

Pros:

Support any children. I think that we can expect new GitHub issues for it once this gets released. We will see if there is enough 👍 to force us to reconsider & revert.
A slightly better transition
Same DOM structure for previous users

discussed

oliviertassinari · 2021-06-12T14:17:51Z

@siriwatknp I have updated #20012 to check [ ] and link this PR. If you could do it for the other efforts, it will be 👌.

siriwatknp added 3 commits June 9, 2021 14:18

remove label wrapper

4692c8b

fix loading button

98cb239

run proptypes

fbc5155

siriwatknp added new feature New feature or request breaking change component: button This is the name of the generic UI component, not the React module! labels Jun 9, 2021

siriwatknp requested review from oliviertassinari, mnajdova and eps1lon June 9, 2021 07:53

siriwatknp commented Jun 9, 2021

View reviewed changes

siriwatknp added 2 commits June 9, 2021 15:00

fix classes

98a80eb

add breaking change docs

48096de

siriwatknp force-pushed the fix/button-remove-label branch from de5b0b9 to 48096de Compare June 9, 2021 08:10

siriwatknp added 2 commits June 9, 2021 15:20

run prettier and docs:formatted

b801c8a

fix test

ea67b47

mnajdova reviewed Jun 9, 2021

View reviewed changes

packages/material-ui/src/Button/Button.test.js Outdated Show resolved Hide resolved

mnajdova reviewed Jun 9, 2021

View reviewed changes

fix typo

89bee5d

mnajdova approved these changes Jun 9, 2021

View reviewed changes

oliviertassinari previously requested changes Jun 9, 2021

View reviewed changes

change transition only center

4f14a27

siriwatknp requested a review from oliviertassinari June 10, 2021 02:53

fix icon transition flash

6aabb2a

oliviertassinari reviewed Jun 10, 2021

View reviewed changes

siriwatknp merged commit edfaadc into mui:next Jun 11, 2021

siriwatknp deleted the fix/button-remove-label branch June 11, 2021 03:41

oliviertassinari mentioned this pull request Jun 12, 2021

[RFC] Material-UI v5 🚀 #20012

Closed

This was referenced Jun 14, 2021

[IconButton] Add size large and update styles #26748

Merged

[IconButton] Remove label span #26801

Merged

oliviertassinari mentioned this pull request Jun 20, 2021

[Button] Disabled loading button text is sometimes transparent #26833

Closed

2 tasks

This was referenced Jun 23, 2021

v5.0.0 alpha.38 #26906

Merged

[BottomNavigation] Remove wrapper from BottomNavigationAction #26923

Merged

[Tabs] Remove unnecessary wrapper from Tab #26926

Merged

eps1lon mentioned this pull request Aug 23, 2021

[material-ui][LoadingButton] Crashes React when Chrome has Translated the Page #27853

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button] Remove label span #26666

[Button] Remove label span #26666

siriwatknp commented Jun 9, 2021 •

edited

Loading

siriwatknp Jun 9, 2021

oliviertassinari Jun 9, 2021

mui-pr-bot commented Jun 9, 2021 •

edited

Loading

siriwatknp Jun 9, 2021

siriwatknp Jun 9, 2021 •

edited

Loading

mnajdova left a comment

mnajdova left a comment

oliviertassinari commented Jun 9, 2021

oliviertassinari left a comment •

edited

Loading

oliviertassinari Jun 9, 2021

siriwatknp commented Jun 10, 2021

oliviertassinari commented Jun 10, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Jun 12, 2021

[Button] Remove label span #26666

[Button] Remove label span #26666

Conversation

siriwatknp commented Jun 9, 2021 • edited Loading

siriwatknp Jun 9, 2021

Choose a reason for hiding this comment

oliviertassinari Jun 9, 2021

Choose a reason for hiding this comment

mui-pr-bot commented Jun 9, 2021 • edited Loading

siriwatknp Jun 9, 2021

Choose a reason for hiding this comment

siriwatknp Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

oliviertassinari commented Jun 9, 2021

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari Jun 9, 2021

Choose a reason for hiding this comment

siriwatknp commented Jun 10, 2021

oliviertassinari commented Jun 10, 2021 • edited Loading

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Jun 12, 2021

siriwatknp commented Jun 9, 2021 •

edited

Loading

mui-pr-bot commented Jun 9, 2021 •

edited

Loading

siriwatknp Jun 9, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Jun 10, 2021 •

edited

Loading

oliviertassinari left a comment •

edited

Loading