mui
[mui-material][button] Loading props added to button
All functionality from loadingButton has been moved to Button in mui-material
closes #42684
- [x] I have followed (at least) the PR section of the contributing guide.
Netlify deploy preview
- docs/data/material/components/button-group/button-group.md
- docs/data/material/components/buttons/buttons.md
- docs/data/material/migration/upgrade-to-v6/upgrade-to-v6.md
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% :heart_eyes:, gzip: -0.46% :heart_eyes: @material-ui/lab: parsed: -0.14% :heart_eyes:, gzip: +0.02%
Bundle size report
Details of bundle changes (Toolpad) Details of bundle changes
Generated by :no_entry_sign: dangerJS against 579e3313e85aeff2b913149ab1b843ffb37082ba
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 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 https://github.com/mui/material-ui/blob/e52c45ee0330678187e8826ab95f7ac383ed006c/docs/data/material/components/buttons/buttons.md?plain=1#L4
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!
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 can continue with this PR since it seems inactive. It will also resolve #31235.
@aarongarciah This is ready for further review. The Argos CI is failing because the 'Fetch data' button was removed from the demo.
@ZeeshanTamboli @aarongarciah I am able to remove the label that wraps the children and it looks fine to me. Would you mind do a final check if I missed anything?
Before: https://codesandbox.io/p/sandbox/nice-volhard-pw7gqg?file=%2Fsrc%2FDemo.tsx%3A12%2C18
After: https://codesandbox.io/p/sandbox/serene-bassi-js4z6v?file=%2Fsrc%2FDemo.tsx%3A22%2C15
Both produce the same visual apprearance.
@siriwatknp what about this? Is it not a problem? https://github.com/mui/material-ui/pull/35198
@siriwatknp what about this? Is it not a problem? #35198
I pushed an alternative
@siriwatknp what about this? Is it not a problem? #35198
I am able to fix this problem without wrapping an extra span. However, the loading indicator wrapper has to always exist (with display: none when not loading).
I think this solution is better than wrapping with span because it won't cause layout shift.
https://github.com/user-attachments/assets/368f0584-8eae-48dd-a222-ad0b37e94353
Confirmed google translate doesn't break: https://codesandbox.io/p/sandbox/material-ui-cra-ts-forked-fdkkn7.
@aarongarciah @siriwatknp I made these changes:
- Removed the button label slot and class: https://github.com/mui/material-ui/pull/42987/commits/a354fd511c8b33ab549d99f4eefd4dc5f84b784a
- Updated the warning text: https://github.com/mui/material-ui/pull/42987/commits/c24da44e758fb821487b0fed76eae6826e898549
- Updated the migration guide: https://deploy-preview-42987--material-ui.netlify.app/material-ui/migration/upgrade-to-v6/#button-with-loading-state
I guess we just need to check if this PR also resolves #43436.
I guess we just need to check if this PR also resolves #43436.
That's about IconButton, I think it's better to add loading to IconButton in this PR. Otherwise, it will be a regression of https://github.com/mui/material-ui/pull/43436.
@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
