material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

[material-ui][Button] Button doesn't have the loading props

Open gtanchak opened this issue 1 year ago • 20 comments

Summary

Add loading Prop to MUI Button Component

Currently, the MUI library offers a LoadingButton component within the @mui/lab package to manage loading states. However, this requires an additional dependency, which might be an overhead for projects where the only use case is managing a loading state for buttons.

Feature Request: To enhance the usability and convenience of the MUI Button component, I propose adding a isLoading prop directly to the existing MUI Button component in the @mui/material package. This addition would streamline the process of managing loading states without requiring the installation of another dependency.

Examples

<Button isLoading={true}>Submit</Button>

Motivation

  1. Reduces Dependencies: Avoid the need for installing the @mui/lab package just for the loading button functionality.
  2. Improves User Experience: Provides a straightforward way to manage loading states for buttons, improving the developer experience.

Search keywords: Button doesn't have the isLoading Props

gtanchak avatar Jun 19 '24 08:06 gtanchak

The LoadingButton will be moved to @mui/material in the next major version, we want new components to live in the lab so that we can experiment and collect feedback before promoting them as stable. @DiegoAndai typically in major versions we were moving the lab components to the @mui/mateiral package. Would it make sense to include this effort for v6? I think we can move the LoadingButton and the Timeline* components

Reduces Dependencies: Avoid the need for installing the @mui/lab package just for the loading button functionality.

See the above section for why we do this

Improves User Experience: Provides a straightforward way to manage loading states for buttons, improving the developer experience.

It was intentional that this is a new component so that we don't increase the bundle for the button component for this specific use-case

mnajdova avatar Jun 19 '24 12:06 mnajdova

@mnajdova Even if the LoadingButton will be moved to @mui/material in the next major version, managing the loading state requires handling two different components. Instead of this, why not let the Button component manage the loading state? I understand you mentioned not wanting to increase the bundle size of the Button component. However, adding a new component like LoadingButton will increase the overall package size of @mui/material, which seems counterproductive.

gtanchak avatar Jun 24 '24 05:06 gtanchak

@mnajdova, the loading use case is common, so we should discuss whether having a separate component would be better. Adding this functionality shouldn't significantly increase the Button size, would it?

Quickly benchmarking, both Chakra UI and Mantine have the loading functionality in the Button component:

  • https://mantine.dev/core/button/#loading-state
  • https://v2.chakra-ui.com/docs/components/button/usage#button-loading-state

What do you think @aarongarciah?

DiegoAndai avatar Jul 01 '24 19:07 DiegoAndai

@DiegoAndai IMO, a separate LoadingButton doesn't make sense, and we should consider adding the loading variant in the Button component.

aarongarciah avatar Jul 02 '24 07:07 aarongarciah

@mnajdova, the loading use case is common, so we should discuss whether having a separate component would be better. Adding this functionality shouldn't significantly increase the Button size, would it?

Quickly benchmarking, both Chakra UI and Mantine have the loading functionality in the Button component:

  • https://mantine.dev/core/button/#loading-state
  • https://v2.chakra-ui.com/docs/components/button/usage#button-loading-state

What do you think @aarongarciah?

@DiegoAndai Yes, even if react-aria, NextUI also maintain loading state in the Button component so adding loading state would be better.

gtanchak avatar Jul 02 '24 07:07 gtanchak

I don't think we should default to adding more features into components if we can split them - these decision will increase the perception that the components are bloated. I don't understand how the DX would be better, when the LoadingButton is exactly the same API as the Button + the loading logic.

For example, our docs do not use any LoadingButton, but it has over 400+ usages of the Button component.

mnajdova avatar Jul 02 '24 15:07 mnajdova

when the LoadingButton is exactly the same API as the Button + the loading logic

What's the point of maintaining two separate components? Having a single one is easier to teach, document and maintain IMO. The idea is that a user should be able to mark a Button as loading without the need to switch to another component.

aarongarciah avatar Jul 02 '24 15:07 aarongarciah

@aarongarciah And even if only for the loading purpose if we need to use different component that would not make sense.

@mnajdova And, also user will not use LoadingButton ever. It is easy and better to write logic inside the Button component.

gtanchak avatar Jul 02 '24 15:07 gtanchak

What's the point of maintaining two separate components?

Bundle size is the main reason. We are loading another component, the CircularProgress, is it worth always loading this as part of the Button bundle always? If yes, we can add this logic and maintain one Button. The idea was, if you need a button that could have loading state, you can use the LoadingButton, if not you can default to the regular Button component. Why would people need to pay the penalty of always having the CircularProgress in their bundle if they never use it. Btw, I don't have strong opinion, I am just sharing the historical reason for it. We should be cautious when making API decisions in general into what is most common vs what is used 3% of the use-cases.

If tomorrow someone asks for a split button, would be add this logic in the Button component or have a different component? -> this was the way of thinking when creating the component :)

mnajdova avatar Jul 02 '24 20:07 mnajdova

For example, our docs do not use any LoadingButton, but it has over 400+ usages of the Button component.

Our docs are not a web app with lots of forms and pending states, so it's natural we don't need a loading button in our docs (maybe in the feedback form and a couple more places?). In the last few years, the collective UX expectations have skyrocketed (thanks to Remix and their push to go back to forms), and users expect to see a pending state whenever an async operation occurs. So, I'd say that adding a loading state to the Button makes total sense for our users, even if the CircularProgress is included for everyone.

Q: Could we look into dynamically importing CircularProgress? I assume doing that at the library level can be tricky because bundlers may treat these differently.

aarongarciah avatar Jul 03 '24 07:07 aarongarciah

Also, as @DiegoAndai pointed out, basically every component library has a loading state in their buttons, not a separate component: Chakra, Mantine, Ant, Radix Themes, Reshaped, shadcn, Next UI, etc.

As a consumer, it would be very confusing to see two components documented with almost the same API and functionality, the only difference being one being for loading states. Makes no sense to me.

aarongarciah avatar Jul 03 '24 07:07 aarongarciah

<Button loading> is a far superior API than <LoadingButton>. It is crazy to have two separate components to represent two states of a single component.

colmtuite avatar Jul 03 '24 10:07 colmtuite

I think we can work on this after the v6 stable, as it would be a new feature, so no rush from my side to include it in the milestone.

If we agree on the approach of the loading prop, we can also deprecate the LoadingButton component.

DiegoAndai avatar Jul 03 '24 21:07 DiegoAndai

Hey, if no one is working on this right now I'd love to pick it up.

Gavin-10 avatar Jul 10 '24 03:07 Gavin-10

Q: Could we look into dynamically importing CircularProgress? I assume doing that at the library level can be tricky because bundlers may treat these differently.

@romgrk just added lazy support for the ripple, but honestly, I wouldn't do it for the loading prop. If we feel strongly that it should be part of the component, no need to complicate the logic

mnajdova avatar Jul 10 '24 15:07 mnajdova

@mnajdova sounds good!

aarongarciah avatar Jul 10 '24 15:07 aarongarciah

Btw, I don't have strong opinion, I am just sharing the historical reason for it. We should be cautious when making API decisions in general into what is most common vs what is used 3% of the use-cases.

I just wanted to make sure in the future we are not blocked on such a minor decisions :) I was just sharing the historical reasons for why we have a different component, if I say "I am not feeling too strong about it" feel free to ignore the reason if it is not strong enough and implement the change :) My comment wasn't a blocker at all from my side. I could probably improve the phrasing I use 😄

mnajdova avatar Jul 10 '24 15:07 mnajdova

@Gavin-10 feel free to start working on it. The implementation should be very similar to what exists today in LoadingButton. Let us know if you get stuck or need help.

aarongarciah avatar Jul 10 '24 15:07 aarongarciah

Hey, I’ve got all the functionality implemented, however I keep getting a timeout error on the ci/circle e2e-website test in the material-color-playground.spec.ts and products-drawer.spec.ts files. If anyone is free at some point would you be able to help me through this failing test. Thank you.

Gavin-10 avatar Jul 19 '24 23:07 Gavin-10

Thanks @Gavin-10! I'll take a look next week. Let's continue the conversation related to the PR in the PR.

aarongarciah avatar Jul 20 '24 20:07 aarongarciah

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] @gtanchak How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

github-actions[bot] avatar Nov 14 '24 12:11 github-actions[bot]

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue. Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

[!NOTE] @gtanchak How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

github-actions[bot] avatar Jan 13 '25 02:01 github-actions[bot]