[LoadingButton] Fix variant default prop not inheriting from MuiButton theme slot
#30126
Conversation
ZeeshanTamboli
added
component: LoadingButton
| @@ -106,7 +106,7 @@ const LoadingButtonLoadingIndicator = styled('div', { | |||
| left: 14, | |||
| }), | |||
| ...(ownerState.loadingPosition === 'start' && | |||
| ownerState.variant === 'text' && { | |||
| (!ownerState.variant || ownerState.variant === 'text') && { | |||
There was a problem hiding this comment.
I'm might be missing something but this change looks KO. If the default Button variant is outlined, then the style of the LoadingButton should also have the outlined version, not the text one.
Going on step deeper, I challenged if it's a regression or the correct behavior #29740 (comment). In any case, my feedback is that I think we should either do nothing or go all the way down. Currently we restore the previous behavior which was lying to developers.
There was a problem hiding this comment.
@oliviertassinari I agree, and I was personally the one that did the change on the previous PR. The problem is that it is a breaking change, and we released it in a minor, so people complained. We can revisit this again in v6, but in my opinion, it is better to let this change land now. Does this make sense?
There was a problem hiding this comment.
Interesting, I see two topics:
- How should we handle bug fixes that are breaking changes? Personally, I think that we can emphasize on the bug fix part. My hypotheses is that for any bug fix, there are people relying on the wrong behavior, we still need to be able to fix bugs.
- Let's say we are working on v6 right now (we are not, but to simplify the problem) what would be the "right" behavior? (I'm asking because it's not clear to me)
There was a problem hiding this comment.
How should we handle bug fixes that are breaking changes? Personally, I think that we can emphasize on the bug fix part. My hypothesis is that for any bug fix, there are people relying on the wrong behavior, we still need to be able to fix bugs.
Agree, the only problem I see with this scenario is that, no one complained (or saw as a bug the behavior that we had before), so if we are changing it, we are kind of fixing a non-existing bug, or at least a bug people are not aware of. In any way, if we actually do this, we should document better in the changelog the fact that the expected behavior changed for the component, so that people can catch it earlier.
There was a problem hiding this comment.
Let's say we are working on v6 right now (we are not, but to simplify the problem) what would be the "right" behavior? (I'm asking because it's not clear to me)
I would add a default value or the variant inside the LoadingButton.
There was a problem hiding this comment.
Should theme.components.MuiButton.defaultProps.variant also impact the default variant of MuiLoadingButton or not
To be honest I am not sure. From user's perspective it should either always impact the prop in the LoadingButton or not. At this moment, it impacts only the props that do not have a default value in the LoadingButton. This is not intuitive.
On one hand, impacting the default props for the LoadingButton is good, as the component is kind of an extension of the Button. But on the other hand, it looks like a hidden side effect. I think we should create an RFC and decide on this generally for all components and implement it consistently for v6.
There was a problem hiding this comment.
@mnajdova, @oliviertassinari, just to be clear: is the conclusion here to merge this PR and think of a better solution for v6?
There was a problem hiding this comment.
@michaldudak sorry yes. I think we should proceed with the PR as it reverts a breaking change that we introduced. I am adding this discussion to our v6 project :)
There was a problem hiding this comment.
To be honest I am not sure. From user's perspective it should either always impact the prop in the LoadingButton or not. At this moment, it impacts only the props that do not have a default value in the LoadingButton. This is not intuitive.
On one hand, impacting the default props for the LoadingButton is good, as the component is kind of an extension of the Button. But on the other hand, it looks like a hidden side effect. I think we should create an RFC and decide on this generally for all components and implement it consistently for v6.
@mnajdova Ok, in this case, I propose we avoid surprises: theme.components.MuiXXX.defaultProps <==> one component. So no side effects on other components.
Now, in practice, what does it mean?
- the expected behavior in [LoadingButton] defaultProps is no longer inherited from MuiButton defaultProps in @mui/lab 5.0.0-alpha.55 version #29740 is wrong. Per the above
- we fixed a bug with [ButtonGroup] Support different elements under ButtonGroup #28645 "Now we need also change default prop value for MuiLoadingButton separately" this guarantees that not only the prop is correct but also the styles.
So personally, I would suggest we close #30126 and #29740. We are good. Is it a breaking change? I would argue that it was partially working before (only the prop inheritance, not the host style), and we didn't intend to have it work this way, nor was it documented or tested => so I think that it's not a breaking change.
There was a problem hiding this comment.
Sounds fair, I left a comment on the issue, I am closing this PR too. @ZeeshanTamboli thanks for the effort on this one.
oliviertassinari
added
the
component: button
|
This is an issue in version 5.8.7, perhaps a regression? |
Fixes #29740