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

Gavin-10 · 2024-07-18T01:33:40Z

All functionality from loadingButton has been moved to Button in mui-material

Closes #31235 (removes the wrong warning)

Preview: https://deploy-preview-42987--material-ui.netlify.app/material-ui/react-button/

All functionality from loadingButton has been moved to Button in mui-material

mui-bot · 2024-07-18T01:53:32Z

Netlify deploy preview

IconButton: parsed: +4.61% , gzip: +3.47%
Alert: parsed: +3.99% , gzip: +3.08%
Autocomplete: parsed: +2.44% , gzip: +1.83%
@material-ui/core: parsed: +0.58% , gzip: +0.40%
LoadingButton: parsed: -1.09% 😍, gzip: -0.46% 😍
@material-ui/lab: parsed: -0.14% 😍, gzip: +0.02%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 579e331

added Icon class to start and end icons, ran prettier, updated loadingButton tests to test Button. LoadingButton tests not to be merged

aarongarciah

Thanks for working on this @Gavin-10! I did a first pass and left some inline comments. Apart from that, two more things:

The LoadingButton component implementation in mui-lab needs to be updated to just use the newly updated Button from mui-material. The Alert component in mui-lab is a good example of how to do this.
The Button usage docs must be updated:
- The examples under the "Loading button" section must be updated to use Button and this section should be moved before the "File upload" section.
- The "Experimental APIs" heading can be removed.
- LoadingButton shouldn't be listed in the API section at the end of the page.

Let me know if you have any doubts or if you need help.

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

test/test-results/.last-run.json

pnpm-lock.yaml

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

packages/mui-lab/src/LoadingButton/LoadingButton.test.js

Button.js variable names changed, extra classes deleted, loading button tests moved to button.test.js, pnpm-lock restored, last-run.json removed (hopefully)

Gavin-10 · 2024-08-22T23:13:00Z

Thank you so much @aarongarciah for the review! I have implemented all the suggested changes and almost ready to push again. I just have two quick questions. To remove the loading button from the API list do I need to just delete the .json and .js file from the API folder or are there more steps I need to do? Last, all of those extra props were generated when I ran pnpm proptypes and I'm just wondering why they need to be removed so I can have a better understanding for future contributions? Thank you so much.

aarongarciah · 2024-08-23T08:59:04Z

To remove the loading button from the API list do I need to just delete the .json and .js file from the API folder or are there more steps I need to do?

To remove LoadingButton from the API list, you just need to remove LoadingButton from the front matter in the corresponding docs markdown file. See

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

Line 4 in e52c45e

components: Button, IconButton, ButtonBase, LoadingButton

Last, all of those extra props were generated when I ran pnpm proptypes

I should have left the comment in the .d.ts file instead of the .js file. pnpm proptypes generates proptypes based on the types defined in the .d.ts file. So if you remove the unwanted props from the .d.ts file, running pnpm proptypes again will remove the proptypes from the .js file.

Every time you make a proptypes change (add, remove, or modify comments) you'll need to run pnpm docs:api so the corresponding .json files consumed by the docs are updated.

Another common script is pnpm docs:typescript, which we run after modifying any demo file: we work on .tsx files and this script generates the corresponding .js file.

Take a look at https://github.com/mui/material-ui/blob/next/CONTRIBUTING.md#sending-a-pull-request if you have doubts and feel free to ping me anytime you get stuck.

Happy coding!

Extra button classes removed, loading button tests removed and docs removed from API list, loading button depreciated.

Gavin-10 · 2024-08-28T03:29:19Z

Everything should be about wrapped up now. I fixed the dependency issues and some minor styling issues with child elements. The test_unit and test_browser tests failing is expected and caused by the CircularProgress not having accessibility labels. I however do not know why test_e2e_website is failing. Once that is resolved though, I believe everything should be ready.

aarongarciah

I did another quick pass. We're currently focused on supporting the v6 release, but I'll get back to this PR once all feedback is tackled. Thanks for working on this!

docs/data/material/components/button-group/LoadingButtonGroup.tsx

docs/data/material/components/button-group/button-group.md

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

docs/data/material/pagesApi.js

packages/mui-lab/src/LoadingButton/LoadingButton.js

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

siriwatknp · 2024-11-13T12:09:53Z

@ZeeshanTamboli pushed the loading prop for IconButton. Here is the demo https://deploy-preview-42987--material-ui.netlify.app/material-ui/react-button/#loading-with-badge

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

ZeeshanTamboli

Can we add tests for IconButton with loading state?

siriwatknp · 2024-11-14T07:22:06Z

Can we add tests for IconButton with loading state?

Added, thanks.

siriwatknp

👍 I'm pumped. It has been 4 years since LoadingButton was introduced!

ZeeshanTamboli

Pushed two commits recently. Looks good to me!

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

oliviertassinari · 2024-11-15T16:14:52Z

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

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


Missing selector per convention

Suggested change

.${badgeClasses.badge} {

&.${badgeClasses.badge} {

oliviertassinari · 2024-11-15T16:17:45Z

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

-import LoadingButton from '@mui/lab/LoadingButton';
+import Button from '@mui/material/Button';


This change goes in opposition to the initial API design decision, no? #21389. We used import LoadingButton from '@mui/lab/LoadingButton';, we didn't use:

import Button from '@mui/material/Button'; <Button unstable_loading={true}>

The reason was in this dangerbot message:

Personally, as a user, I wouldn't want to use the Material UI Button if it loads a progress component that is different from the one that I want to use because it's bundle size bloat.

Was this change discussed? How do we feel about it?

Happy to move forward with a loading prop though.

I think a <Button loading> is better than having <Button> and <LoadingButton>. I guess 9/10 projects would need a loading button so it's fine to bundle CircularProgress to the Button.

Yes, this was discussed. Having a loading prop in Button is universally established and I think it's preferable to merge LoadingButton and Button to reduce API surface and confusion (why having 2 components with almost the same exact API?), even if it means an increase in the bundle size.

Q: Is there a way we could dynamically import the progress component that would be compatible with all bundlers? Are dynamic imports universally supported?

Personally, as a user, I wouldn't want to use the Material UI Button if it loads a progress component that is different from the one that I want to use because it's bundle size bloat.

This also happens with component styles. Users load our components, which have all styles built-in and are included in the bundle, to later override them.

Q: Is there a way we could dynamically import the progress component that would be compatible with all bundlers? Are dynamic imports universally supported?

@aarongarciah MUI X does this https://github.com/mui/mui-x/blob/a4481b6a661849d1a33b41aad86970a1e212ca69/packages/x-data-grid-premium/src/hooks/features/export/serializer/excelSerializer.ts#L24. It seems to work for them:

Screen.Recording.2024-11-19.at.16.34.51.mov

In the CJS the output is https://unpkg.com/browse/@mui/[email protected]/hooks/features/export/serializer/excelSerializer.js so it only works for ESM.

This wouldn't work for us here, since we would also need an async component, not React 18 friendly.

This also happens with component styles. Users load our components, which have all styles built-in and are included in the bundle, to later override them.

Yeah. Ok, so maybe it's fine. As long as we move to have the source to be clean to copy and paste from the npm package to a project codebase, then, a developer (e.g. me) would eject, and remove all the stuff unneeded, once it truly becomes a problem.

I have checked a couple of design system, they seem to all go with the loading prop pattern. e.g. https://polaris.shopify.com/components/actions/button. So yeah, assuming that we don't expect most people to wrap the MUI component to create their own system, but to either

use it as is with theme customization

or to decompose + recompose with a wrapper, this makes sense.

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

packages/mui-lab/src/LoadingButton/LoadingButton.js

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

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

oliviertassinari · 2024-11-19T15:43:30Z

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

+  const loader = (
+    <ButtonLoadingIndicator className={classes.loadingIndicator} ownerState={ownerState}>
+      {loading && loadingIndicator}
+    </ButtonLoadingIndicator>
+  );


Can we make this conditional? I don't think this can work to render an empty DOM node when loading={false}:

https://deploy-preview-42987--material-ui.netlify.app/material-ui/react-button/#basic-button. DOM size has an impact on performance, buttons that are likely to be rendered in the x100 in a list. Same applies with emotion, another styled() mounting point is more slowness to the page. The most important might be that this can communicate a feeling of "bloat" to developers.

At the very minimum, I think we need a comment to explain why it's here. As a developer looking at the dev tools then the source to try to understand this, I would be: "What?! Let's delete that" I wouldn't know the context around Google translate #35198.

If it's conditional, it won't fix #35198.

I don't think this can fly

May I ask why? what's the issue of it?

If it's conditional, it won't fix

@siriwatknp I mean, what I don't understand is why are we not using the same fix that we used in #35198? Why are we always rendering an extra styled() element even when loading={false}?

May I ask why? what's the issue of it?

I have tried to cover the problem with the initial comment: "DOM size has an impact on performance, buttons that are likely to be rendered in the x100 in a list. Same applies with emotion, another styled() mounting point is more slowness to the page. The most important might be that this can communicate a feeling of "bloat" to developers."

What do you think about those?

…utton (mui#42987)" This reverts commit 7ad8893.

oliviertassinari · 2024-11-26T13:28:33Z

PR was reverted in #44478.

Gavin McGuinness added 2 commits July 17, 2024 21:27

Loading props added to button

7f2f33a

All functionality from loadingButton has been moved to Button in mui-material

ran pnpm deduplicate

aa09045

Gavin-10 marked this pull request as draft July 18, 2024 01:49

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 18, 2024

Gavin McGuinness added 3 commits July 17, 2024 23:18

Imports reformated and eslint failure fixed

7369f21

missing files added

4459f08

More ci/circle issues resolved

451a3a1

added Icon class to start and end icons, ran prettier, updated loadingButton tests to test Button. LoadingButton tests not to be merged

aarongarciah self-requested a review July 20, 2024 20:10

ZeeshanTamboli added component: button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Jul 21, 2024

aarongarciah reviewed Aug 21, 2024

View reviewed changes

aarongarciah self-assigned this Aug 21, 2024

Review suggestions implemented

4ddca3f

Button.js variable names changed, extra classes deleted, loading button tests moved to button.test.js, pnpm-lock restored, last-run.json removed (hopefully)

Docs updated and loading button depreciated

a9cac91

Gavin McGuinness and others added 7 commits August 23, 2024 10:48

Updates to API, Button, and LoadingButton

f045e16

Extra button classes removed, loading button tests removed and docs removed from API list, loading button depreciated.

Resolving dependency issues

e77cc6f

Resolving dependency issues again

6379283

Deduplicating pnpm-lock file

6402d77

Restoring old pnpm-lock file

56449f4

Resolving dependency issues

5186aa9

Manually changed pnpm-lock.yaml file

293ecdc

Gavin-10 marked this pull request as ready for review August 25, 2024 20:52

Gavin-10 added 2 commits August 27, 2024 21:58

Button Group Docs Updated

a3f279c

Child alignment issues fixed

5b429c6

ZeeshanTamboli requested a review from aarongarciah September 12, 2024 14:25

aarongarciah reviewed Sep 12, 2024

View reviewed changes

siriwatknp added 3 commits November 13, 2024 17:19

add loading to IconButton

d0ce7f3

run proptypes

ad5e9c7

run docs:ts:format

739299b

siriwatknp requested a review from ZeeshanTamboli November 13, 2024 12:09

aarongarciah reviewed Nov 13, 2024

View reviewed changes

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

ZeeshanTamboli reviewed Nov 13, 2024

View reviewed changes

siriwatknp added 2 commits November 14, 2024 14:09

separate badge from loading demo

e6ad5c1

add tests for IconButton loading

723d36d

siriwatknp requested review from ZeeshanTamboli, mnajdova and aarongarciah November 14, 2024 07:22

siriwatknp approved these changes Nov 14, 2024

View reviewed changes

aarongarciah approved these changes Nov 14, 2024

View reviewed changes

ZeeshanTamboli added 3 commits November 14, 2024 14:08

fix link

0de27c4

add loading style for styleOverrides

b472bb9

Merge branch 'master' into loading-button

579e331

ZeeshanTamboli approved these changes Nov 14, 2024

View reviewed changes

siriwatknp merged commit 7ad8893 into mui:master Nov 14, 2024
22 checks passed

Gavin-10 deleted the loading-button branch November 14, 2024 15:39

oliviertassinari reviewed Nov 15, 2024

View reviewed changes

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

oliviertassinari reviewed Nov 15, 2024

View reviewed changes

siriwatknp mentioned this pull request Nov 19, 2024

[Button] Fix CircularProgress import #44473

Merged

1 task

aarongarciah added the component: icon button This is the name of the generic UI component, not the React module! label Nov 19, 2024

oliviertassinari reviewed Nov 19, 2024

View reviewed changes

siriwatknp added a commit to siriwatknp/material-ui that referenced this pull request Nov 20, 2024

Revert "[material-ui][Button] Add loading feature to Button and IconB…

767a9bc

…utton (mui#42987)" This reverts commit 7ad8893.

oliviertassinari mentioned this pull request Nov 26, 2024

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

Merged

siriwatknp mentioned this pull request Dec 3, 2024

[Button] Add loading prop #44637

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Gavin-10 commented Jul 18, 2024 •

edited by oliviertassinari

Loading

mui-bot commented Jul 18, 2024 •

edited

Loading

aarongarciah left a comment

Gavin-10 commented Aug 22, 2024

aarongarciah commented Aug 23, 2024

Gavin-10 commented Aug 28, 2024

aarongarciah left a comment

siriwatknp commented Nov 13, 2024

ZeeshanTamboli left a comment

siriwatknp commented Nov 14, 2024

siriwatknp left a comment •

edited

Loading

ZeeshanTamboli left a comment •

edited

Loading

oliviertassinari Nov 15, 2024 •

edited

Loading

oliviertassinari Nov 15, 2024

siriwatknp Nov 19, 2024

aarongarciah Nov 19, 2024

oliviertassinari Nov 19, 2024 •

edited

Loading

oliviertassinari Nov 19, 2024 •

edited

Loading

siriwatknp Nov 19, 2024

siriwatknp Nov 19, 2024

oliviertassinari Nov 20, 2024 •

edited

Loading

oliviertassinari commented Nov 26, 2024

		import LoadingButton from '@mui/lab/LoadingButton';
		import Button from '@mui/material/Button';

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

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

Conversation

Gavin-10 commented Jul 18, 2024 • edited by oliviertassinari Loading

mui-bot commented Jul 18, 2024 • edited Loading

Netlify deploy preview

Bundle size report

aarongarciah left a comment

Choose a reason for hiding this comment

Gavin-10 commented Aug 22, 2024

aarongarciah commented Aug 23, 2024

Gavin-10 commented Aug 28, 2024

aarongarciah left a comment

Choose a reason for hiding this comment

siriwatknp commented Nov 13, 2024

ZeeshanTamboli left a comment

Choose a reason for hiding this comment

siriwatknp commented Nov 14, 2024

siriwatknp left a comment • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli left a comment • edited Loading

Choose a reason for hiding this comment

oliviertassinari Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Nov 15, 2024

Choose a reason for hiding this comment

siriwatknp Nov 19, 2024

Choose a reason for hiding this comment

aarongarciah Nov 19, 2024

Choose a reason for hiding this comment

oliviertassinari Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

siriwatknp Nov 19, 2024

Choose a reason for hiding this comment

siriwatknp Nov 19, 2024

Choose a reason for hiding this comment

oliviertassinari Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

oliviertassinari commented Nov 26, 2024

Gavin-10 commented Jul 18, 2024 •

edited by oliviertassinari

Loading

mui-bot commented Jul 18, 2024 •

edited

Loading

siriwatknp left a comment •

edited

Loading

ZeeshanTamboli left a comment •

edited

Loading

oliviertassinari Nov 15, 2024 •

edited

Loading

oliviertassinari Nov 19, 2024 •

edited

Loading

oliviertassinari Nov 19, 2024 •

edited

Loading

oliviertassinari Nov 20, 2024 •

edited

Loading