Skip to content
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

[Button] Add loading support #7223

Closed
thupi opened this issue Jun 23, 2017 · 27 comments · Fixed by #21389
Closed

[Button] Add loading support #7223

thupi opened this issue Jun 23, 2017 · 27 comments · Fixed by #21389
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request priority: important This change can make a difference

Comments

@thupi
Copy link

thupi commented Jun 23, 2017

Description

I have a hard time indicating that the website is making a server request and the user might wait and at the same time prevent the user from clicking login again. Some of our users repeatedly click login if the server responds slowly.

It cloud be an excellent feature if the button could expect at "loading" prop. When the button is passed loading={true} it should disable interaction and indicate it is loading.

I am currently using material-ui@next in a project which is planned to be put into production soon :-) Great progress :-)!

Images & references

image

image

Screenshots is taken from https://vuetifyjs.com/components/buttons (Loaders section).

@oliviertassinari oliviertassinari added the new feature New feature or request label Jun 23, 2017
@oliviertassinari

This comment has been minimized.

@oliviertassinari oliviertassinari changed the title [Button] Loading state for buttons in next branch [Button] Loading state for buttons Jun 23, 2017
@oliviertassinari oliviertassinari added component: button This is the name of the generic UI component, not the React module! v1.x.x good first issue Great for first contributions. Enable to learn the contribution process. labels Jun 23, 2017
@thupi

This comment has been minimized.

@oliviertassinari

This comment has been minimized.

@thupi

This comment has been minimized.

@imerkle
Copy link

imerkle commented Jun 24, 2017

you can create a function that disables button and another enables. and wrap your ajax request function like

1 call the disable button
2 do ajax ->
2 callback of ajax -> enable button

and in the disable/enable functions you can further do whatever customisation you need using like adding loaders etc.

@thupi
Copy link
Author

thupi commented Jun 24, 2017

@dsslimshaddy Nice solution.

At the moment i have made a wrapper component that injects a span into the button which add the loader. Futher more it prevents the onClick method from being called :-) ! That might work for my need at the moment. Thanks for the suggestion :-)

@vritant24
Copy link

Hi, if no one else has taken it, I'd like to try and implement this feature.

@thupi
Copy link
Author

thupi commented Jun 28, 2017

@vritant24 Hi, that sounds awesome :-) !

@vritant24
Copy link

On which buttons would this feature be applied on?

@oliviertassinari
Copy link
Member

There is only one button on the v1-alpha branch.

@thupi
Copy link
Author

thupi commented Jul 14, 2017

@vritant24 Did you take this task :-) ? Otherwise i might look a bit into how this could be solved :-)

@vritant24
Copy link

@thupi yes you can go ahead and look into it. I've had to move on to other work

@esseswann
Copy link
Contributor

I think if the feature is implemented we could check if the onClick function is async and then transition to loading state by default.

@imerkle
Copy link

imerkle commented Aug 8, 2017

here's something i wrapped https://www.webpackbin.com/bins/-Kr01wKU_hD4ILcj8IPJ

@AuroreM

This comment has been minimized.

@slavab89
Copy link
Contributor

You could do something yourself which seems to work pretty well
Create a component that wraps a button and does something like

<Button>
  {!this.props.loading ?
    <CircularProgress size={14} /> : // Size 14 works pretty well
    <Typography>Click Me</Typography>}
</Button>

If you like to style your own button or dont really want to have the inner span for whatever reason, you could always use ButtonBase

@oliviertassinari

This comment has been minimized.

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation and removed new feature New feature or request labels Sep 13, 2017
@CamdenKo
Copy link

Hey there, should I add @slavab89 's implementation to the documentation?

@oliviertassinari
Copy link
Member

@CamdenKo Sure, it would be great :)

@jankalfus
Copy link

Be careful that using LinearProgress inside of Button produces invalid HTML as LinearProgress is implemented using a div. And divs can't be placed inside of buttons, it's not valid HTML.

@oliviertassinari
Copy link
Member

@jankalfus component="span" should solve this problem :).

@oliviertassinari oliviertassinari added new feature New feature or request and removed docs Improvements or additions to the documentation good first issue Great for first contributions. Enable to learn the contribution process. labels Dec 16, 2019
@oliviertassinari oliviertassinari changed the title [Button] Loading state for buttons [Button] Add loading support Dec 16, 2019
@oliviertassinari
Copy link
Member

We have tried to solve this problem in the core in #17810 but it didn't go into the core. I think that it would be great to resume the effort.

@jankalfus
Copy link

@oliviertassinari Hmm, doesn't seem like the component prop is available in either CircularProgress or LinearProgress: https://github.com/mui-org/material-ui/blob/6ced014170c18eefa5549f2a165881a73d94f943/packages/material-ui/src/CircularProgress/CircularProgress.js

@oliviertassinari
Copy link
Member

@jankalfus It sounds like we have found a compelling use case for adding such component prop, in case you want to lead this effort, I think that we should accept a pull request for it :).

@mbrookes
Copy link
Member

mbrookes commented Dec 17, 2019

Does CircularProgress need to be a child of the button? The current docs examples overlay the progress indicator over the button.

https://material-ui.com/components/progress/#interactive-integration

#17810 used the progress indicator as a startIcon, but looking at the spec the current approach in the docs seems closer: https://material.io/components/progress-indicators/#circular-progress-indicators

"[Circular progress indicators] can be applied directly to a surface, such as a button or card."

See also: "Integrating with actions" for more details and a video.

(The other thing I noticed is that what we renamed to static has replaced the more complex animation that is currently used for circular determinate.)

@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 17, 2019

Does CircularProgress need to be a child of the button?

I think that it has the potential to give the best implementation. Without this approach, an extra div needs to be placed around the button, which might break the current display mode, e.g. inline vs block.

@mbrookes Thanks for the link to the specification, I believe it's what Vuetify implements, it looks great. Keeping the same shape of the button seems an important property (layout stability).

@alvamanu
Copy link

alvamanu commented Feb 19, 2020

The tricky part is not the if / else isloading. If you have an array of rows and those rows have the custom component button, say, <ButtonWithLoader/>, it's very tricky to tell which button has been clicked. I currently have the issue that when initiating an axios call, I update the oncall prop to true, but since the <ButtonWithLoader/> I've created has been duplicated, the loader shows in each button.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: button This is the name of the generic UI component, not the React module! new feature New feature or request priority: important This change can make a difference
Projects
None yet
Development

Successfully merging a pull request may close this issue.