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] Change LoadingButton prop pending to loading #21593

Closed
adamsr123 opened this issue Jun 27, 2020 · 22 comments · Fixed by #25874
Closed

[Button] Change LoadingButton prop pending to loading #21593

adamsr123 opened this issue Jun 27, 2020 · 22 comments · Fixed by #25874
Labels
breaking change component: button This is the name of the generic UI component, not the React module! discussion ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Milestone

Comments

@adamsr123
Copy link
Contributor

adamsr123 commented Jun 27, 2020

I'm very happy to see the new pre-release v5.0.0-alpha.1. I noticed something:

  • The Autocomplete component has a prop called loading.
  • The LoadingButton component has a prop called pending.

For convention, I think all components should use the word loading. Also it feels it makes sense after the component name

@adamsr123 adamsr123 added design: material This is about Material Design, please involve a visual or UX designer in the process status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 27, 2020
@adamsr123 adamsr123 changed the title Change LoadingButton prop pending to loading [LoadingButton] Change LoadingButton prop pending to loading Jun 27, 2020
@oliviertassinari oliviertassinari added discussion and removed design: material This is about Material Design, please involve a visual or UX designer in the process status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jun 27, 2020
@oliviertassinari
Copy link
Member

Thanks for raising this API inconsistency, much appreciated. I believe the closest prior discussion we have is in #21389 (comment) with @mnajdova and @eps1lon.

@oliviertassinari oliviertassinari added the component: button This is the name of the generic UI component, not the React module! label Jun 27, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 29, 2020

Looking at the semantic, we have:

The use of pending echos back to https://reactjs.org/docs/concurrent-mode-patterns.html#the-three-steps.

I don't have any strong preference, I think that we can settle this with a vote, the end goal is to make it intuitive to developers.

  • 👍 for loading
  • 🎉 for pending

@adamsr123
Copy link
Contributor Author

adamsr123 commented Jun 29, 2020

I don't have any strong preference, I think that we can settle this with a vote, the end goal is to make it intuitive to developers.

  • 👍 for loading
  • 🎉 for pending

@oliviertassinari Good idea, I still think that the word loading is much more used and specially in React components.

UI framework examples: React Bootstrap, React spinners, React Semantic-UI, Ant

@oliviertassinari
Copy link
Member

It seems that we can go into the direction proposed by @adamsr123

@oliviertassinari oliviertassinari added new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted and removed discussion labels Jul 1, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 1, 2020

The Autocomplete component has a prop called loading.
The LoadingButton component has a prop called pending.

If you look at the component you'll see that these props are in fact describing different UI patterns. Naming them differently is very much intended. Before reading the concurrent mode docs I would've probably agreed. But it's now apparent to me that we should distinguish these patterns in naming so that they aren't misused.

@oliviertassinari oliviertassinari added discussion and removed new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Jul 1, 2020
@ryancogswell
Copy link
Contributor

If you look at the component you'll see that these props are in fact describing different UI patterns.

@eps1lon It isn't clear to me that "pending" is any more accurate than "loading" in this case. Neither prop name is ideal for this case. Unlike Autocomplete, which can itself be in a "loading" state, this button is indicating the state of something else -- the button itself isn't "pending" or "loading", rather it might indicate a pending transition or that something else is loading.

As far as the concurrent mode docs you referenced, though the distinction between "pending" and "loading" is useful in discussing the details of how to leverage useTransition and Suspense appropriately, I'm not convinced that the distinction is relevant here. I would expect the LoadingButton may be used in any scenario where the button triggers something to be loaded and where the developer does not want to allow the button to be clicked again during any part of the time of that loading. The button might be triggering something which will eventually cause the button to go away (as in the concurrent mode example) or it could be triggering changes in another portion of the screen (e.g. an area above or below the button) and the button might return to its previous state when the loading is complete.

When the button is triggering changes in another part of the screen, that portion of the screen being updated could be in either the "pending" state (i.e. still showing the previous contents) or "loading" state (e.g. showing a skeleton representation of what is coming) with regard to the concurrent mode terminology. For the display of the button, we don't care about that granularity -- the button doesn't have three states ("pending", "loading", "normal"), just two. In either of the pending and loading states, it is still the case that something is loading, it is just a difference in what is being shown in the area that will be updated when the loading completes.

The most accurate name would be a SomethingElseIsLoadingButton component with a somethingElseIsLoading prop, but I don't think people are likely to be confused by the current (and much less tedious) LoadingButton name into thinking that the button itself is loading, and having the prop be loading to align with the component name makes sense to me.

@ryancogswell ryancogswell reopened this Jul 1, 2020
@mnajdova
Copy link
Member

mnajdova commented Jul 1, 2020

If you look at the component you'll see that these props are in fact describing different UI patterns. Naming them differently is very much intended. Before reading the concurrent mode docs I would've probably agreed. But it's now apparent to me that we should distinguish these patterns in naming so that they aren't misused.

I would have to agree eith @eps1lon on this one, we should follow react’s terms for describing the patterns which are already defined in their implementation. The button itself is not in loading state, bit it can show a pending state. The name of the component is suggesting that it can be used in use cases when something will be loading on the page, but the state it has is pending. In addition having a LoadingButton with loading={false} as prop looks really awkward in my opinion...

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 11, 2020

While naming is a hard problem, let's take a step back and answer this question: What does a great name accomplish? I think that the priorities and important points are:

  1. Intuitiveness. One of the best leverage to create joy in users when using a product is to reduce, as much as possible, the friction of using it. The more intuitive the API is, the less time you would have to spend on the documentation finding what you are looking for. I think that a great way to accomplish this is to have names that match how most people would name the items themselves.
  2. Consistency. It doesn’t matter what one personal preference might be, it’s more important that the code everybody is sharing uses consistent names. I think that this element makes all the difference hence put in the first position. We, humans, can't live in the complexity of the world without building simplification models. Remember the first time you had to drive a car? Overwhelming, information coming from every direction, how can I handle that? Over time, your brain has creates models and simplification on how to react to different events, what to pay attention to on the road. I think that the same happens when using a library of UI components, our brain is wired to simplify thing, once we see a pattern, we expect it to be present every time. When the API breaks the consistency, it feels unintuitive and awkward.
  3. Communicate intent, to everybody that will read the code. It's important that the names we choose make sense to the most people.

I think that we should go with "loading":

  • It's what people voted for here. This vote shouldn't be confused with what most popular in the community. Here, we present arguments from both sides and let people choose, informed.
  • It matches the name of the component. A likely path for users: "Alright, I need to display a loader. I'm going to use LoadingButton component, now, what's the prop to trigger it? Oh, it must be loading". Loading is consistent with the name of the component.
  • It's ubiquitous in the community. We have already agreed on this observation.
  • pending is nich. React defines the pending state in the concurrent docs as:

When we useTransition, React will let us “stay” on the previous screen — and show a progress indicator there. We call that a Pending state.

https://reactjs.org/docs/concurrent-mode-patterns.html#preferred-pending-skeleton-complete

I agree with the point of @ryancogswell in #21593 (comment), It feels that "loading" includes the meaning of "pending" with a broader one. I think that the objective of the prop should be to signal to the users that the UI isn't idle, something is going on behind the scene. "busy" could almost be a better name if taking the 3. Communicate intent track alone.

@eps1lon
Copy link
Member

eps1lon commented Jul 11, 2020

the button itself isn't "pending" or "loading", rather it might indicate a pending transition or that something else is loading.

Exactly. It will always tell if something else is pending. Sometimes this will indicate loading. This makes pending the better name.

It feels that "loading" includes the meaning of "pending" with a broader one.

It is a special case of a pending transition. It is not a more general case. This is not an argument for loading.

It's ubiquitous in the community. We have already agreed on this observation.

Again, this is a fallacy. Just because we used to do something does not mean it is the right thing to do. Concurrent patterns are new and it is just wrong to compare these to past terminology that have no explainer. We just used loading if we load data. Reusing this terminology for transitions sets the UI up for failure.

It's what people voted for here

Could you engage with my argument instead of repeating the ones I already refuted?

Consistency

You agree with my point. In a React ecosystem with concurrent mode "pending" is the better name because of consistency.

Intuitiveness

You're repeating consistency arguments.

Communicate intent

Also supporting my argument. The intent is to trigger a transition. It might load data or trigger a suspense boundary in the broadest sense.

@ryancogswell
Copy link
Contributor

@eps1lon Are you arguing for pending only for the prop name or do you also think the component should be called PendingButton? If you are fine with the component name of LoadingButton, why do you feel differently about the prop?

It will always tell if something else is pending. Sometimes this will indicate loading.

In what case is a transition pending when it isn't because something (code or data) is loading?

It is a special case of a pending transition.

I disagree. I don't feel like either is broader. "pending" focuses on the state of transition, "loading" refers to why the transition is pending. Why haven't we transitioned yet? Because we're still loading something. So the question becomes, is it more intuitive to refer to the transition state or is it more intuitive to refer to the activity that is the cause of the transition state. I would argue that developers will ask, "what is the property I need to set to show that something is still loading?" more readily than asking "what is the property I need to set to indicate that my transition is in a pending state?"

Reusing this terminology for transitions sets the UI up for failure.

In what way does using the term "loading" set the UI up for failure? If someone isn't aware of the new capabilities provided by concurrent mode in React and how to use them, Material-UI using "pending" for this prop name isn't going to make them more aware. And if a developer does understand the nuances of concurrent mode, Material-UI using "loading" for this prop name is not going to lead them astray.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 11, 2020

Just because we used to do something does not mean it is the right thing to do

@eps1lon Agree, the argument is only that changing people's habits is hard, leveraging what they already know is less effort (which is a valuable property). It doesn't have an implication of being right. The majority can be wrong.

You're repeating consistency arguments.

The two are close but different. consistency !== intuitiveness. A counterexample: we could be using "foo" for the prop on the whole codebase making it consistent, and yet, it won't make it intuitive. I think that consistency is necessary for making an API intuitive but isn't sufficient.

The intent is to trigger a transition.

My assumption was different, I believe the transition state is second-order, it's a concern that leaks outside of the responsibility of the button, it's the application (or the suspense component parent of the button) that is in a pending state, not the button. What's displayed in the button when the prop is true? A loader, tree dots, a circular progress.

@oliviertassinari oliviertassinari added breaking change ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Jul 14, 2020
@oliviertassinari oliviertassinari added this to the v5 milestone Jul 14, 2020
@tomByrer
Copy link

I vote for pending prop since LoadingButton may be used for far more things than just loading.
Prop name consistency is a good idea though.

@eps1lon
Copy link
Member

eps1lon commented Jul 24, 2020

I think that consistency is necessary for making an API intuitive but isn't sufficient.

You understand that you're saying that an inconsistent API can never be intuitive? We would need to name every prop the same even if they do different things. That's definitely wrong. Consistency might be a characteristic of an intuitive API but consistency is never a goal in and of itself.

What's displayed in the button when the prop is true? A loader, tree dots, a circular progress.

So we name it showLoader then? If you care about what is displayed then you don't use the ing suffix.

If someone isn't aware of the new capabilities provided by concurrent mode in React and how to use them

The API of concurrent mode doesn't matter. The mental model does.

@ryancogswell
Copy link
Contributor

Consistency might be a characteristic of an intuitive API but consistency is never a goal in and of itself.

@eps1lon To me, the most compelling consistency argument is regarding being consistent with itself (which is why I asked before whether you also wanted to change the name of the component). For the moment, we have decided to call this LoadingButton, but aside from the component name, nowhere in the code (props, CSS classes) does it refer to "load", "loader", or "loading". Instead we have "pending" and "pendingIndicator". If this was just a prop on Button, I wouldn't really care whether the prop name was loading or pending, but it seems extremely unintuitive to call the component LoadingButton and then use completely separate terminology for all the aspects that are the reason for it being called LoadingButton.

I actually like "pending indicator" as a generic term for the loader/spinner, but for reasons I can't fully explain, I think PendingButton sounds silly -- I think largely because it sounds too much like the button is the thing that is "pending" (yes, I realize "LoadingButton" has this same problem). I do like PendingIndicatorButton. It's a bit verbose, but I think it is much clearer. With this name, I would find it very intuitive that it also has a pendingIndicator prop for providing that element and a pending prop for turning on the pending indicator and disabling the button. Even though it is more verbose, I think pendingPosition should be pendingIndicatorPosition. If we did these changes, I feel the button's purpose/functionality would be clear from its name and that the corresponding props would be intuitive and unsurprising.

@tomByrer
Copy link

tomByrer commented Jul 24, 2020

@ryancogswell I have a separate discussion & demo for renaming the JSX tag here. Yes, 'pending' captures a wider use-case.

BTW @oliviertassinari Thank you for being open to discussion. "Naming things is hard."

@eps1lon
Copy link
Member

eps1lon commented Jul 27, 2020

#21903 is a good example why "consistency" is not an argument for names. We use different words in language to describe different concepts. And pending vs loading fall in the same category as round vs. circle

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 27, 2020

What about: we try <PendingButton pending />, add "loading" in the description of the demo/component for SEO, and see how that goes?

It seems to come down to:

  • "loading" better matches the name the crowd is expecting for the component. A guess: 80% of the use cases for a pending button is to wait for the result of a PUT/POST/GET request, "loading data" from the network. I think it's why "loading" is more popular.
  • The usage of "pending" is more abstract and accurate, it can include a broader set of use cases (the 20% other).

Arguably we could also rename <Autocomplete loading /> to <Autocomplete pending />, but in this case, the loading use case is probably not 80% of the need, more like 98%. It makes less sense to use a more abstracted term.

@tomByrer
Copy link

we try , add "loading"

Sound fair to me, thanks for the consideration.

@mbrookes mbrookes reopened this Oct 15, 2020
@oliviertassinari oliviertassinari changed the title [LoadingButton] Change LoadingButton prop pending to loading [Button] Change LoadingButton prop pending to loading Jan 27, 2021
@landon912
Copy link

landon912 commented Apr 8, 2021

Does this proposal include renaming the props pendingIndicator and pendingPosition or is this strictly related to the pending prop?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 8, 2021

@landon912 Intuitively, I would propose that both pendingIndicator and pendingPosition are renamed at the same time.

@Divyansh-sysadmin-creative

Hi Devs,
facing issue with buttonLoading on ios device. loading symbol not visible on button. Same works well on android device and mac ios. Please guide with solution

@mnajdova
Copy link
Member

mnajdova commented Oct 3, 2022

Hi Devs,
facing issue with buttonLoading on ios device. loading symbol not visible on button. Same works well on android device and mac ios. Please guide with solution

@Divyansh-sysadmin-creative please open an issue with a reproduction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: button This is the name of the generic UI component, not the React module! discussion ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
9 participants