[mui-material][button] Loading props added to button #42987

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

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

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

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

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

Netlify deploy preview

@material-ui/core: parsed: +Infinity% , gzip: +Infinity%
@mui/joy: parsed: +Infinity% , gzip: +Infinity%
@material-ui/lab: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Autocomplete: parsed: +Infinity% , gzip: +Infinity%
@material-ui/unstyled: parsed: +Infinity% , gzip: +Infinity%
Autocomplete: parsed: +Infinity% , gzip: +Infinity%
TextField: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Select: parsed: +Infinity% , gzip: +Infinity%
SpeedDialAction: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Menu: parsed: +Infinity% , gzip: +Infinity%
Tooltip: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Tooltip: parsed: +Infinity% , gzip: +Infinity%
SwipeableDrawer: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Drawer: parsed: +Infinity% , gzip: +Infinity%
TabList: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/Snackbar: parsed: +Infinity% , gzip: +Infinity%
SpeedDial: parsed: +Infinity% , gzip: +Infinity%
Pagination: parsed: +Infinity% , gzip: +Infinity%
Alert: parsed: +Infinity% , gzip: +Infinity%
@mui/joy/MenuButton: parsed: +Infinity% , gzip: +Infinity%
and 271 more changes

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5b429c6

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.

aarongarciah · 2024-08-21T13:10:25Z

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

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


These 3 props shouldn't be here. These are part of the classes prop which and they're already handled in buttonClasses.ts

aarongarciah · 2024-08-21T13:10:50Z

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

This file shouldn't be commited.

aarongarciah · 2024-08-21T13:11:31Z

pnpm-lock.yaml

No dependency changes should be included in this PR. Please undo changes in this file.

aarongarciah · 2024-08-21T13:34:34Z

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

+const LoadingButtonLoadingIndicator = styled('span', {
+  name: 'MuiLoadingButton',


Suggested change

const LoadingButtonLoadingIndicator = styled('span', {

name: 'MuiLoadingButton',

const ButtonLoadingIndicator = styled('span', {

name: 'MuiButton',

aarongarciah · 2024-08-21T13:35:58Z

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

    size,
    type,
    variant,
  };

  const classes = useUtilityClasses(ownerState);

+  const loadingButtonLoadingIndicator = loading ? (


Suggested change

const loadingButtonLoadingIndicator = loading ? (

const buttonLoadingIndicator = loading ? (

aarongarciah · 2024-08-21T13:37:54Z

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

+  /**
+   * Styles applied to the endIcon element if `loading={true}` and `loadingPosition="end"`.
+   */
+  endIconLoadingEnd: PropTypes.string,


This prop shouldn't be here. These is part of the classes prop which is already handled in buttonClasses.ts

aarongarciah · 2024-08-21T13:38:31Z

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

+  /**
+   * Styles applied to the startIcon element if `loading={true}` and `loadingPosition="start"`.
+   */
+  startIconLoadingStart: PropTypes.string,


This prop shouldn't be here. These is part of the classes prop which is already handled in buttonClasses.ts

aarongarciah · 2024-08-21T13:44:44Z

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

    size = 'medium',
    startIcon: startIconProp,
    type,
    variant = 'text',
    ...other
  } = props;

+  const id = useId(idProp);
+  const loadingIndicator = loadingIndicatorProp ?? (
+    <CircularProgress aria-labelledby={id} aria-label={`${children}`} color="inherit" size={16} />


aria-label={${children}} will produce be equal to "[object Object]" if a React element is passed as children. Let's use the exact same implementation that exists in LoadingButton for now. See example: https://codesandbox.io/p/sandbox/brave-panini-xm3vsv?file=/src/App.js:4,16

Suggested change

<CircularProgress aria-labelledby={id} aria-label={`${children}`} color="inherit" size={16} />

<CircularProgress aria-labelledby={id} color="inherit" size={16} />

aarongarciah · 2024-08-21T13:47:03Z

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

All of LoadingButton tests should be moved to the Button component in mui-material. This file shouldn't exist once the refactor is done.

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[mui-material][button] Loading props added to button #42987

[mui-material][button] Loading props added to button #42987

Gavin-10 commented Jul 18, 2024

mui-bot commented Jul 18, 2024 •

edited

Loading

aarongarciah left a comment

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

aarongarciah Aug 21, 2024

Gavin-10 commented Aug 22, 2024

aarongarciah commented Aug 23, 2024

Gavin-10 commented Aug 28, 2024

		const LoadingButtonLoadingIndicator = styled('span', {
		name: 'MuiLoadingButton',

	const loadingButtonLoadingIndicator = loading ? (
	const buttonLoadingIndicator = loading ? (

	<CircularProgress aria-labelledby={id} aria-label={`${children}`} color="inherit" size={16} />
	<CircularProgress aria-labelledby={id} color="inherit" size={16} />

[mui-material][button] Loading props added to button #42987

Are you sure you want to change the base?

[mui-material][button] Loading props added to button #42987

Conversation

Gavin-10 commented Jul 18, 2024

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

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

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

Choose a reason for hiding this comment

Gavin-10 commented Aug 22, 2024

aarongarciah commented Aug 23, 2024

Gavin-10 commented Aug 28, 2024

mui-bot commented Jul 18, 2024 •

edited

Loading