-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mui-material][button] Loading props added to button #42987
base: master
Are you sure you want to change the base?
Conversation
All functionality from loadingButton has been moved to Button in mui-material
Netlify deploy preview
@material-ui/core: parsed: +Infinity% , gzip: +Infinity% Bundle size reportDetails of bundle changes (Toolpad) |
added Icon class to start and end icons, ran prettier, updated loadingButton tests to test Button. LoadingButton tests not to be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 updatedButton
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.
- The examples under the "Loading button" section must be updated to use
Let me know if you have any doubts or if you need help.
/** | ||
* 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These 3 props shouldn't be here. These are part of the classes
prop which and they're already handled in buttonClasses.ts
test/test-results/.last-run.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file shouldn't be commited.
pnpm-lock.yaml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No dependency changes should be included in this PR. Please undo changes in this file.
const LoadingButtonLoadingIndicator = styled('span', { | ||
name: 'MuiLoadingButton', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const LoadingButtonLoadingIndicator = styled('span', { | |
name: 'MuiLoadingButton', | |
const ButtonLoadingIndicator = styled('span', { | |
name: 'MuiButton', |
size, | ||
type, | ||
variant, | ||
}; | ||
|
||
const classes = useUtilityClasses(ownerState); | ||
|
||
const loadingButtonLoadingIndicator = loading ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const loadingButtonLoadingIndicator = loading ? ( | |
const buttonLoadingIndicator = loading ? ( |
/** | ||
* Styles applied to the endIcon element if `loading={true}` and `loadingPosition="end"`. | ||
*/ | ||
endIconLoadingEnd: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop shouldn't be here. These is part of the classes
prop which is already handled in buttonClasses.ts
/** | ||
* Styles applied to the startIcon element if `loading={true}` and `loadingPosition="start"`. | ||
*/ | ||
startIconLoadingStart: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop shouldn't be here. These is part of the classes
prop which is already handled in buttonClasses.ts
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} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
<CircularProgress aria-labelledby={id} aria-label={`${children}`} color="inherit" size={16} /> | |
<CircularProgress aria-labelledby={id} color="inherit" size={16} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
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. |
To remove
I should have left the comment in the .d.ts file instead of the .js file. Every time you make a proptypes change (add, remove, or modify comments) you'll need to run Another common script is 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.
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. |
All functionality from loadingButton has been moved to Button in mui-material
closes #42684