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

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

Open
gtanchak opened this issue Jun 19, 2024 · 21 comments · Fixed by #42987 · May be fixed by #44637
Open

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

gtanchak opened this issue Jun 19, 2024 · 21 comments · Fixed by #42987 · May be fixed by #44637
Assignees
Labels
component: LoadingButton The React component. new feature New feature or request package: lab Specific to @mui/lab ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@gtanchak
Copy link

gtanchak commented Jun 19, 2024

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 gtanchak added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 19, 2024
@mnajdova
Copy link
Member

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 mnajdova added package: lab Specific to @mui/lab component: LoadingButton The React component. and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 19, 2024
@gtanchak
Copy link
Author

@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.

@DiegoAndai
Copy link
Member

@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:

What do you think @aarongarciah?

@aarongarciah
Copy link
Member

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

@gtanchak
Copy link
Author

gtanchak commented Jul 2, 2024

@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:

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.

@mnajdova
Copy link
Member

mnajdova commented Jul 2, 2024

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.

@aarongarciah
Copy link
Member

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.

@gtanchak
Copy link
Author

gtanchak commented Jul 2, 2024

@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.

@mnajdova
Copy link
Member

mnajdova commented Jul 2, 2024

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 :)

@aarongarciah
Copy link
Member

aarongarciah commented Jul 3, 2024

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
Copy link
Member

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.

@colmtuite
Copy link
Contributor

<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.

@DiegoAndai DiegoAndai added the new feature New feature or request label Jul 3, 2024
@DiegoAndai
Copy link
Member

DiegoAndai commented Jul 3, 2024

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 DiegoAndai self-assigned this Jul 3, 2024
@DiegoAndai DiegoAndai added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 3, 2024
@Gavin-10
Copy link
Contributor

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

@aarongarciah aarongarciah changed the title [material-ui][Button] - Button doesn't have the loading props [material-ui][Button] Button doesn't have the loading props Jul 10, 2024
@mnajdova
Copy link
Member

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

@aarongarciah
Copy link
Member

@mnajdova sounds good!

@mnajdova
Copy link
Member

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 😄

@aarongarciah
Copy link
Member

@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.

@Gavin-10
Copy link
Contributor

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.

@aarongarciah
Copy link
Member

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

Copy link

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.

@ZeeshanTamboli ZeeshanTamboli linked a pull request Dec 6, 2024 that will close this issue
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: LoadingButton The React component. new feature New feature or request package: lab Specific to @mui/lab ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
7 participants