[lab][LoadingButton] LoadingButton now inherits props from ButtonGroup

[lhilgert9]

Closes #34415 by using the ButtonGroupContext in the LoadingButton. It works like in the normal Button.

[mui-bot]

Netlify deploy preview

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

@material-ui/lab: parsed: +0.42% , gzip: +0.35%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 01512fa

[ZeeshanTamboli]

We should add tests, and I also suggest adding documentation that demonstrates Loading Buttons inside ButtonGroup.

[lhilgert9]

@ZeeshanTamboli do you think its better to place the documentation at the ButtonGroup or the Button page?

And what tests did you have in mind?

And another question: Why is the LoadingButton actually in @mui/lab and not in @mui/material-ui

[ZeeshanTamboli]

@ZeeshanTamboli do you think its better to place the documentation at the ButtonGroup or the Button page?

And what tests did you have in mind?

And another question: Why is the LoadingButton actually in @mui/lab and not in @mui/material-ui

@lhilgert9, let's start by questioning whether we truly need this feature. Issue #34415 has been open for a year without any requests or upvotes. Since you've created the PR, can you clarify your specific use case for it? Implementing this feature will increase maintenance overhead.

[lhilgert9]

@lhilgert9, let's start by questioning whether we truly need this feature. Issue #34415 has been open for a year without any requests or upvotes. Since you've created the PR, can you clarify your specific use case for it? Implementing this feature will increase maintenance overhead.

@ZeeshanTamboli, I personally use the ButtonGroup very often in my projects and also the LoadingButton to show the status of a server request. So far I have to manually pass the stylings of the ButtonGroup to the LoadingButton every time. I think that this issue #34415 doesn't have so many upvotes, because you can fix the bug quite easily by adding the stylings manually. But I think this small change will save a lot of work in the future.

[mnajdova]

The changes make sense to me 👍 Let's add a test in the LoadinButton component that will verify that if it is used in a ButtonGroup it will inherit the props passed from this component. Also, let's include update one of the button group demos to show a loading button.

[lhilgert9]

The changes make sense to me 👍 Let's add a test in the LoadinButton component that will verify that if it is used in a ButtonGroup it will inherit the props passed from this component. Also, let's include update one of the button group demos to show a loading button.

@mnajdova I've added two tests and a demo to the docs.

Preview: https://deploy-preview-39679--material-ui.netlify.app/material-ui/react-button-group/#loading-button

[lhilgert9]

@mnajdova What about this PR? I had actually fixed all the errors or had I forgotten something?

[mnajdova]

Looks good, left one final comment.

[lhilgert9]

@mnajdova Okay, I have fixed that. Should be ready to merge now👍🏽.

[ZeeshanTamboli]

Not sure why CI is failing. Will try again later.

[lhilgert9]

@ZeeshanTamboli It looks like the error is the same as here, because in my last commit the node version which is used for CI is 18.18.0: https://github.com/mui/material-ui/actions/runs/6943811139/job/18889666611?pr=39679 and in your current commit it uses 18.19.0: https://github.com/mui/material-ui/actions/runs/7141802378/job/19472684424?pr=39679.
The error was not noticed because the old version was still used for the test when the version was upgraded: https://github.com/mui/material-ui/actions/runs/7073941852/job/19254444675?pr=40080.

(You can see the used Node version when clicking Use Node.js 18.x and then in line 9 Environment details)

I opened #40173 to better talk about the error

[ZeeshanTamboli]

Looks good now. The validate-declarations script command line was updated in #39947. But looks like we'll still be reverting the node version in #40187.

[milotoor]

Shouldn't this have been accompanied by a peer dependency version bump for @mui/material? It introduces the dependency on the ButtonGroupContext, but as of v5.14.20 that's not exported from the ButtonGroup module. The export was made available in v5.15.0, so the current peer-dep of "@mui/material": ">=5.10.11" is far too permissive.

[ZeeshanTamboli]

Shouldn't this have been accompanied by a peer dependency version bump for @mui/material? It introduces the dependency on the ButtonGroupContext, but as of v5.14.20 that's not exported from the ButtonGroup module. The export was made available in v5.15.0, so the current peer-dep of "@mui/material": ">=5.10.11" is far too permissive.

I see, but what would be the advantage? @mui/lab is always in alpha.

[milotoor]

I see, but what would be the advantage? @mui/lab is always in alpha.

Well, in my opinion if the package is going to specify a minimum peer dependency version at all it might as well be a correct one. It would've saved me a minor headache yesterday when I went to upgrade the lab package after dependabot suggested doing so. It did not suggest upgrading @mui/material (or the other packages) and I did not think to check for a concurrent release. If the peerDependency had been updated NPM would've alerted me to the incompatibility between the packages. Instead I happily merged the dependabot PR and only found out some hours later that the application was throwing runtime errors everywhere the LoadingButton is used.

I resolved the issue by upgrading to v5.15.0 after a brief investigation.

[ZeeshanTamboli]

Well, in my opinion if the package is going to specify a minimum peer dependency version at all it might as well be a correct one. It would've saved me a minor headache yesterday when I went to upgrade the lab package after dependabot suggested doing so. It did not suggest upgrading @mui/material (or the other packages) and I did not think to check for a concurrent release. If the peerDependency had been updated NPM would've alerted me to the incompatibility between the packages. Instead I happily merged the dependabot PR and only found out some hours later that the application was throwing runtime errors everywhere the LoadingButton is used.

I resolved the issue by upgrading to v5.15.0 after a brief investigation.

I agree with you. @mnajdova Maybe we should take a look at this for future releases.

[mnajdova]

Coming a bit late to the conversation, but that was a good observation @milotoor , we should update the peer dependency, if we haven't already. @ZeeshanTamboli could you take a look at this?

[oliviertassinari]

Breaking change label added, it was one.