[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.45%
LoadingButton: parsed: -0.73% 😍, gzip: -0.13% 😍
@material-ui/lab: parsed: 0.00% 😍, gzip: +0.12%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 978a40e

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

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.

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.

DiegoAndai · 2024-12-09T19:33:20Z

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

@@ -364,13 +524,22 @@ const Button = React.forwardRef(function Button(inProps, ref) {
    endIcon: endIconProp,
    focusVisibleClassName,
    fullWidth = false,
+    id: idProp,


What's id's prop required for?

It's for labeling the loader:

const loadingIndicator = loadingIndicatorProp ?? ( <CircularProgress aria-labelledby={id} color="inherit" size={16} /> );

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.

DiegoAndai · 2024-12-09T19:54:49Z

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

+
+{{"demo": "LoadingIconButton.js"}}
+
+### Badge


I wouldn't include the loading prop in the Badge's demo. It should be only about the Badge.

…/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.

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

mnajdova reviewed Dec 3, 2024

View reviewed changes

packages/mui-material/src/Button/Button.js Show resolved Hide resolved

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

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

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

DiegoAndai left a comment

DiegoAndai Dec 9, 2024

siriwatknp Dec 11, 2024

DiegoAndai Dec 9, 2024

siriwatknp Dec 11, 2024

DiegoAndai Dec 9, 2024

siriwatknp Dec 11, 2024

siriwatknp commented Dec 11, 2024


		{{"demo": "LoadingIconButton.js"}}

		### Badge

[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

DiegoAndai left a comment

Choose a reason for hiding this comment

DiegoAndai Dec 9, 2024

Choose a reason for hiding this comment

siriwatknp Dec 11, 2024

Choose a reason for hiding this comment

DiegoAndai Dec 9, 2024

Choose a reason for hiding this comment

siriwatknp Dec 11, 2024

Choose a reason for hiding this comment

DiegoAndai Dec 9, 2024

Choose a reason for hiding this comment

siriwatknp Dec 11, 2024

Choose a reason for hiding this comment

siriwatknp commented Dec 11, 2024

[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