[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 is 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.4.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.55% , gzip: +3.60%
Alert: parsed: +4.02% , gzip: +3.10%
Autocomplete: parsed: +2.45% , gzip: +1.86%
@material-ui/core: parsed: +0.64% , gzip: +0.45%
LoadingButton: parsed: -0.78% 😍, gzip: -0.15% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 15192ab

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

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

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.

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

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

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.

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

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

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

…/button-loading3

DiegoAndai · 2025-01-02T19:02:56Z

@siriwatknp The last thing from my side is this comment which hasn't been fixed yet: #44637 (comment)

siriwatknp · 2025-01-03T02:58:51Z

@siriwatknp The last thing from my side is this comment which hasn't been fixed yet: #44637 (comment)

I misunderstood your point but it's fixed now.

ZeeshanTamboli

Just these two things. Rest looks good.

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

docs/data/material/migration/upgrade-to-v6/upgrade-to-v6.md

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

Co-authored-by: Zeeshan Tamboli <[email protected]> Co-authored-by: Diego Andai <[email protected]> Signed-off-by: Siriwat K <[email protected]>

…/button-loading3

DiegoAndai · 2025-01-09T15:18:34Z

This looks good to me from the technical perspective 👍🏼

I would like for @aarongarciah to approve this from the product perspective. If he does, then we're ready to merge.

I'll be OOO until Wednesday. If Aaron approves from the product perspective, don't wait for me, you can just merge.

aarongarciah

Nice work! Left some small comments on the docs

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

…/button-loading3

The loading behavior is now built-in: mui/material-ui#44637.

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

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

packages/mui-material/src/Button/buttonClasses.ts Outdated Show resolved Hide resolved

docs/data/material/components/buttons/buttons.md Show resolved Hide resolved

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

ZeeshanTamboli reviewed Dec 17, 2024

View reviewed changes

docs/data/material/components/buttons/IconButtonWithBadge.tsx Outdated Show resolved Hide resolved

docs/data/material/components/buttons/buttons.md Outdated Show resolved Hide resolved

run docs:api

d3cf66a

DiegoAndai reviewed Dec 20, 2024

View reviewed changes

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

siriwatknp added 4 commits December 23, 2024 10:29

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

d50b95f

…/button-loading3

simplify loading styles

32d7170

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

d7b36ed

…/button-loading3

update docs

602691d

siriwatknp requested review from ZeeshanTamboli and DiegoAndai January 2, 2025 03:16

update available version

02abe9d

add loadingPosition* classes to button

0e87a59

siriwatknp force-pushed the feat/button-loading3 branch from d3d0926 to 0e87a59 Compare January 3, 2025 02:22

ZeeshanTamboli reviewed Jan 8, 2025

View reviewed changes

docs/data/material/components/buttons/buttons.md Outdated Show resolved Hide resolved

docs/data/material/migration/upgrade-to-v6/upgrade-to-v6.md Outdated Show resolved Hide resolved

DiegoAndai reviewed Jan 8, 2025

View reviewed changes

packages/mui-material/src/Button/buttonClasses.ts Outdated Show resolved Hide resolved

siriwatknp and others added 2 commits January 9, 2025 17:53

Apply suggestions from code review

5d4986e

Co-authored-by: Zeeshan Tamboli <[email protected]> Co-authored-by: Diego Andai <[email protected]> Signed-off-by: Siriwat K <[email protected]>

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

38d6c8f

…/button-loading3

DiegoAndai requested a review from aarongarciah January 9, 2025 15:18

aarongarciah approved these changes Jan 9, 2025

View reviewed changes

DiegoAndai approved these changes Jan 9, 2025

View reviewed changes

siriwatknp added 4 commits January 10, 2025 08:54

apply suggestion

222af9f

run docs:api

8bbabb7

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

4b60f03

…/button-loading3

update doc

15192ab

siriwatknp mentioned this pull request Jan 10, 2025

[material-ui] Fix slots typing for Tooltip and StepLabel #44985

Merged

1 task

siriwatknp merged commit a76cf2e into mui:master Jan 13, 2025
22 checks passed

johnnyomair added a commit to vivid-planet/comet that referenced this pull request Jan 16, 2025

Use normal Button instead of FeedbackButton

fc6f338

The loading behavior is now built-in: mui/material-ui#44637.

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

siriwatknp commented Dec 11, 2024

DiegoAndai Dec 16, 2024

siriwatknp commented Dec 17, 2024

DiegoAndai commented Jan 2, 2025

siriwatknp commented Jan 3, 2025

ZeeshanTamboli left a comment

DiegoAndai commented Jan 9, 2025

aarongarciah left a comment

[Button] Add loading prop #44637

[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

siriwatknp Dec 6, 2024

Choose a reason for hiding this comment

DiegoAndai left a comment

Choose a reason for hiding this comment

siriwatknp commented Dec 11, 2024

DiegoAndai Dec 16, 2024

Choose a reason for hiding this comment

siriwatknp commented Dec 17, 2024

DiegoAndai commented Jan 2, 2025

siriwatknp commented Jan 3, 2025

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

DiegoAndai commented Jan 9, 2025

aarongarciah left a 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