[Button] Add loading prop support #17810
Conversation
Details of bundle changes.Comparing: 31d3008...326e15f
|
| loading: { | ||
| pointerEvents: 'none', // Disable link interactions | ||
| cursor: 'default', | ||
| opacity: 0.4, |
There was a problem hiding this comment.
Same value as the disabled rating.
| @@ -260,7 +262,7 @@ const ButtonBase = React.forwardRef(function ButtonBase(props, ref) { | |||
| className={clsx( | |||
| classes.root, | |||
| { | |||
| [classes.disabled]: disabled, | |||
| [classes.disabled]: disabledProp, | |||
There was a problem hiding this comment.
I had to introduce a private loading prop to disable the interaction without applying the disabled CSS. I don't like that, but I can't think of a better approach.
| variant="contained" | ||
| color="primary" | ||
| loading | ||
| startIcon={<CircularProgress color="inherit" size={16} />} |
There was a problem hiding this comment.
Most implementations bundle the circular directly in the button. I'm not confident this is best from a bundle size perspective. I'm wondering. It might also complexify the API to expose in case people want to change how the loading is displayed.
There was a problem hiding this comment.
This is a general statement not targeted at anyone particular: I'm getting weary of this "props everything" approach. React introduced "component based ui development" not "props based ui development". It's ok to add another LoadableButton where one could flip the switch and we provide the spinner.
oliviertassinari
added
component: button
| @@ -162,6 +162,12 @@ export const styles = theme => ({ | |||
| focusVisible: {}, | |||
| /* Pseudo-class applied to the root element if `disabled={true}`. */ | |||
| disabled: {}, | |||
| /* Styles applied to the root element if `loading={true}`. */ | |||
| loading: { | |||
There was a problem hiding this comment.
It's not actually related to loading (it's only used in this context). What this actually conveys is that the button is "deactivated" or "not actionable" or "temporary disabled". loading implies that there is another indicator.
As I said below I'd be perfectly fine with introducing a separate component that overrides Button properly.
There was a problem hiding this comment.
As I said below I'd be perfectly fine with introducing a separate component that overrides Button properly.
The loading state is transient, how would you switch between the loading and non-loading state with a new component? In this regard, I think that a switch of a prop has the potential to be simpler to use.
There was a problem hiding this comment.
I don't think transient applies to a loading state or any UI in that matter.
But even then you can still have the loading prop which then actually conveys the proper meaning by switching styles and adding an indicator.
oliviertassinari
added
PR: needs revision
|
I'm closing, it doesn't seem to be a very frequent user request (while I expect it to be frequently needed) but it seems to require more effort to reach a consensus at least, we have: https://material-ui.com/components/progress/#interactive-integration. Thanks for the review guys. |
Well, actually, BusyButton might be a good enough solution, but I would rather focus on different problems. If somebody wants to resume this effort 👍. |
Benchmark
The implementation was benchmarked with the following sources. I have noticed that the loading and disabled states are often styled independently.
evergreen
blueprint
vuetify
cosmos
antd
element
sancho
semantic ui
quasar
baseui
atlaskit
chakra
orbit