[Button] Add `loading` prop #44637

siriwatknp · 2024-12-03T07:53:13Z

Preview: https://deploy-preview-44637--material-ui.netlify.app/material-ui/react-button/#loading-2

Context

This PR build on top of the reverted #42987 by making the default value of loading to be null to fix the problem with Google Translation Crash without introducing overhead of the loading wrapper to the existing projects.

When the prop is boolean, a span always present as a wrapper regardless of the loading state:

<button>
  <span style={{ display: 'contents' }}></span> // still present even though loading is false
  text
</button>

The reasons to go with this approach instead of wrapping children with a span are:

No breaking change
No overhead for existing projects
Small overhead when loading because the loading wrapper is not build with styled.

Note

When this PR is merged, the next release should be v6.2.0

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

This reverts commit 232555b.

siriwatknp · 2024-12-03T07:55:57Z

@oliviertassinari ~~Based on your comment, are you okay with the new prop?~~ Please see this comment instead. I will update this PR if you are okay with @mnajdova suggestion.

mui-bot · 2024-12-03T07:56:25Z

Netlify deploy preview

IconButton: parsed: +4.52% , gzip: +3.56%
Alert: parsed: +3.99% , gzip: +3.03%
Autocomplete: parsed: +2.44% , gzip: +1.83%
@material-ui/core: parsed: +0.66% , gzip: +0.46%
LoadingButton: parsed: -0.71% 😍, gzip: -0.11% 😍
@material-ui/lab: parsed: +0.01% , gzip: +0.13%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against d3cf66a

mnajdova · 2024-12-03T11:52:41Z

This PR build on top of the reverted #42987 by adding a new prop enableLoadingWrapper to specifically fix the problem with Google Translation Crash.

Wouldn't it be better to automatically add the span if the loading prop has a boolean value (true/false), and don't add it if it has a nullish value. We can explain in the demos the usage.

siriwatknp · 2024-12-04T03:19:58Z

This PR build on top of the reverted #42987 by adding a new prop enableLoadingWrapper to specifically fix the problem with Google Translation Crash.

Wouldn't it be better to automatically add the span if the loading prop has a boolean value (true/false), and don't add it if it has a nullish value. We can explain in the demos the usage.

That's a nice suggestion. If I understand correctly, there is no need for the new prop, right. The default value of loading is null which will not render the extra span by default. When loading={boolean}, the span is always there.

mnajdova · 2024-12-05T10:06:23Z

That's a nice suggestion. If I understand correctly, there is no need for the new prop, right. The default value of loading is null which will not render the extra span by default. When loading={boolean}, the span is always there.

Yes, the downside is that a component cannot alternative between supporting loading or not (at least in terms of working with Google translate). I think that's nice compromise.

DiegoAndai · 2024-12-05T12:58:50Z

The default value of loading is null which will not render the extra span by default. When loading={boolean}, the span is always there.

I like this approach 👍🏼 let's add a short explanation in the docs as well.

…/button-loading3

This reverts commit f317e69.

siriwatknp · 2024-12-06T03:25:06Z

@mnajdova @DiegoAndai Ready for review.

ZeeshanTamboli · 2024-12-06T07:00:15Z

@siriwatknp I’ve linked two issues in the description that should be closed when this PR merges, the same as in #44637.

siriwatknp · 2024-12-06T13:14:16Z

packages/mui-material/src/Button/Button.js

      {...other}
      classes={classes}
    >
      {startIcon}
+      {enableLoadingWrapper ? (
+        // use plain HTML span to minimize the runtime overhead
+        <span className={classes.loadingWrapper} style={{ display: 'contents' }}>


DiegoAndai

Hey Jun! Thanks for pickling this one back up again.

I think the loadingPosition needs some work, see these examples: https://codesandbox.io/p/sandbox/44637-feedback-1-wtf45k

In these examples users would expect the loading indicator to position itself correctly.

packages/mui-material/src/Button/Button.js

DiegoAndai · 2024-12-09T19:42:17Z

packages/mui-material/src/Button/buttonClasses.ts

+  loadingIndicatorCenter: string;
+  /** Styles applied to the loadingIndicator element if `loadingPosition="start"`. */
+  loadingIndicatorStart: string;
+  /** Styles applied to the loadingIndicator element if `loadingPosition="end"`. */
+  loadingIndicatorEnd: string;
+  /** Styles applied to the endIcon element if `loading={true}` and `loadingPosition="end"`. */
+  endIconLoadingEnd: string;
+  /** Styles applied to the startIcon element if `loading={true}` and `loadingPosition="start"`. */
+  startIconLoadingStart: string;


This goes against having atomic classes. I think a better approach would be having:

loadingPositionCenter, loadingPositionStart, loadingPositionEnd classes applied to the root depending on the loadingPosition prop.

Which can be combined with other classes, for example

.loadingPositionStart .loadingIndicator { ... } instead of loadingIndicatorStart

.loadingPositionStart .startIcon { ... } instead of startIconLoadingStart

Reducing these 5 classes into 3.

We should try to follow the rule of having one class per prop-value combination.

That's a good point. Removed.

docs/data/material/components/buttons/buttons.md

…/button-loading3

siriwatknp · 2024-12-11T03:16:29Z

think the loadingPosition needs some work, see these examples: https://codesandbox.io/p/sandbox/44637-feedback-1-wtf45k

The root cause is that there is no icon to provide the space for the loading. I fixed this by render a loadingIconPlaceholder slot when there is no icon on that position.

DiegoAndai · 2024-12-16T19:37:40Z

packages/mui-material/src/Button/buttonClasses.ts

+  /** Styles applied to the loadingIndicator element if `loadingPosition="center"`. */
+  loadingIndicatorCenter: string;
+  /** Styles applied to the loadingIndicator element if `loadingPosition="start"`. */
+  loadingIndicatorStart: string;
+  /** Styles applied to the loadingIndicator element if `loadingPosition="end"`. */
+  loadingIndicatorEnd: string;


These should be removed and replaced by

loadingPositionCenter, loadingPositionStart, loadingPositionEnd, applied to the root. This would allow to do

.loadingPositionCenter .loadingIndicator { ... } // instead of loadingIndicatorCenter // but also .loadingPositionCenter .loadingWrapper { ... } // or .loadingPositionCenter .icon { ... }

This follows the prop-value class combination.

Good point, fixed.

@siriwatknp the loadingPositionCenter, loadingPositionStart, and loadingPositionEnd classes should be applied to the root. Otherwise, it won't be possible to compose it with the icon, loadingWrapper, or loadingIconPlaceholder classes.

DiegoAndai · 2024-12-16T19:41:36Z

packages/mui-material/src/Button/Button.js

      {...other}
      classes={classes}
    >
      {startIcon}
+      {enableLoadingWrapper ? (
+        // use plain HTML span to minimize the runtime overhead
+        <span className={classes.loadingWrapper} style={{ display: 'contents' }}>


Should we move style={{ display: 'contents' }} as well?

packages/mui-material/src/Button/Button.js

…/button-loading3

siriwatknp · 2024-12-17T07:23:07Z

Should we move style={{ display: 'contents' }} as well?

@DiegoAndai If you meant to move it to be styled components, I added a comment above it to keep that element as plain HTML for least overhead. Its purpose is to prevent google translate error, not for customization because it has display: contents.

ZeeshanTamboli · 2024-12-17T07:20:22Z

docs/data/material/components/buttons/IconButtonWithBadge.tsx

+import ShoppingCartIcon from '@mui/icons-material/ShoppingCartOutlined';
+
+const CartBadge = styled(Badge)`
+  .${badgeClasses.badge} {


Suggested change

.${badgeClasses.badge} {

&.${badgeClasses.badge} {

ZeeshanTamboli · 2024-12-17T07:22:41Z

docs/data/material/components/buttons/buttons.md

@@ -113,12 +113,45 @@ Use `color` prop to apply theme color palette to component.

 {{"demo": "IconButtonColors.js"}}

+### Loading
+
+Starting from [`v6.2.0`](https://github.com/mui/material-ui/releases/tag/v6.2.0), use `loading` prop to set icon buttons in a loading state and disable interactions.


Just a note to update the version here and in other places before merging.

DiegoAndai · 2024-12-20T15:35:02Z

packages/mui-material/src/Button/Button.js

+    {
+      props: {
+        loadingPosition: 'start',
+        size: 'small',
+      },
+      style: {
+        left: 10,
+      },
+    },
+    {
+      props: ({ loadingPosition, size }) => loadingPosition === 'start' && size !== 'small',
+      style: {
+        left: 14,
+      },
+    },


Would applying the base styles first work?

{ props: { loadingPosition: 'start' }, style: { left: 14, }, }, { props: { loadingPosition: 'start', size: 'small', }, style: { left: 10, }, },

I think this would make it more readable and maintainable. What do you think?

(The same applies to the other 'small' size styles applied elsewhere in this PR.

siriwatknp added 2 commits December 3, 2024 13:40

Revert "[material-ui][Button] Revert Button loading (mui#44478)"

52f690e

This reverts commit 232555b.

add enableLoadingWrapper

f317e69

siriwatknp added new feature New feature or request component: button This is the name of the generic UI component, not the React module! labels Dec 3, 2024

siriwatknp requested review from oliviertassinari and DiegoAndai December 3, 2024 07:53

siriwatknp added 4 commits December 6, 2024 09:45

Merge branch 'master' of https://github.com/mui/material-ui into feat…

ecdab2b

…/button-loading3

Revert "add enableLoadingWrapper"

baf6709

This reverts commit f317e69.

set loading default to null

ce1206a

add warning

92a33b5

siriwatknp requested a review from mnajdova December 6, 2024 03:22

siriwatknp commented Dec 6, 2024

View reviewed changes

DiegoAndai reviewed Dec 9, 2024

View reviewed changes

siriwatknp added 4 commits December 11, 2024 09:50

Merge branch 'master' of https://github.com/mui/material-ui into feat…

33543e7

…/button-loading3

remove combined classes

6d404da

remove loading from badge demo

92d392e

fix loading position without icon

5ce445f

siriwatknp requested a review from DiegoAndai December 11, 2024 03:16

run docs:api

978a40e

DiegoAndai reviewed Dec 16, 2024

View reviewed changes

mui deleted a comment from mnajdova Dec 16, 2024

siriwatknp added 3 commits December 17, 2024 13:57

Merge branch 'master' of https://github.com/mui/material-ui into feat…

82f9be8

…/button-loading3

remove order

65cfdbe

fix button loading position class

c9911ff

ZeeshanTamboli reviewed Dec 17, 2024

View reviewed changes

run docs:api

d3cf66a

DiegoAndai reviewed Dec 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Button] Add `loading` prop #44637

[Button] Add `loading` prop #44637

siriwatknp commented Dec 3, 2024 •

edited by ZeeshanTamboli

Loading

siriwatknp commented Dec 3, 2024 •

edited

Loading

mui-bot commented Dec 3, 2024 •

edited

Loading

mnajdova commented Dec 3, 2024

siriwatknp commented Dec 4, 2024

mnajdova commented Dec 5, 2024

DiegoAndai commented Dec 5, 2024

siriwatknp commented Dec 6, 2024

ZeeshanTamboli commented Dec 6, 2024

siriwatknp Dec 6, 2024

DiegoAndai left a comment

DiegoAndai Dec 9, 2024

siriwatknp Dec 11, 2024

siriwatknp commented Dec 11, 2024

DiegoAndai Dec 16, 2024

siriwatknp Dec 17, 2024

DiegoAndai Dec 20, 2024

DiegoAndai Dec 16, 2024

siriwatknp commented Dec 17, 2024

ZeeshanTamboli Dec 17, 2024

ZeeshanTamboli Dec 17, 2024

DiegoAndai Dec 20, 2024

[Button] Add loading prop #44637

Are you sure you want to change the base?

[Button] Add loading prop #44637

Conversation

siriwatknp commented Dec 3, 2024 • edited by ZeeshanTamboli Loading

Context

Note

siriwatknp commented Dec 3, 2024 • edited Loading

mui-bot commented Dec 3, 2024 • edited Loading

Netlify deploy preview

Bundle size report

mnajdova commented Dec 3, 2024

siriwatknp commented Dec 4, 2024

mnajdova commented Dec 5, 2024

DiegoAndai commented Dec 5, 2024

siriwatknp commented Dec 6, 2024

ZeeshanTamboli commented Dec 6, 2024

Choose a reason for hiding this comment

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp commented Dec 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siriwatknp commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Button] Add `loading` prop #44637

[Button] Add `loading` prop #44637

siriwatknp commented Dec 3, 2024 •

edited by ZeeshanTamboli

Loading

siriwatknp commented Dec 3, 2024 •

edited

Loading

mui-bot commented Dec 3, 2024 •

edited

Loading