[Button] Remove label span by siriwatknp · Pull Request #26666 · mui/material-ui

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

Continue #24222

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

  ...(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 type: new feature Expand the scope of the product to solve a new problem. breaking change Introduces changes that are not backward compatible. scope: button Changes related to the button. labels Jun 9, 2021

siriwatknp requested review from eps1lon, mnajdova and oliviertassinari 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

ZeeshanTamboli mentioned this pull request Nov 9, 2024

[material-ui][Button] Add loading feature to Button #42987

Merged

oliviertassinari mentioned this pull request Nov 26, 2024

[material-ui][Button] Revert Button loading #44478

Merged

Uh oh!

Conversation

siriwatknp commented Jun 9, 2021 • edited by oliviertassinari Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siriwatknp Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

mui-pr-bot commented Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

siriwatknp Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

siriwatknp Jun 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

mnajdova left a comment

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Jun 9, 2021

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari Jun 9, 2021

Choose a reason for hiding this comment

Uh oh!

siriwatknp commented Jun 10, 2021

Uh oh!

oliviertassinari commented Jun 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oliviertassinari commented Jun 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

siriwatknp commented Jun 9, 2021 •

edited by oliviertassinari

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