search

vmware-clarity / core

Clarity is a scalable, accessible, customizable, open-source design system built with web components. Works with any JavaScript framework, created for enterprises, and designed to be inclusive.
https://clarity.design
MIT License
166 stars 42 forks source link

setting disabled attribute while loadingState is "loading" has no effect on cds-button #129

Closed EndzeitBegins closed 2 years ago

EndzeitBegins commented 2 years ago

Describe the bug

The disabled attribute of cds-button(from @cds/angular) is ignored, when it is set while the loadingState is loading.

We recently migrated a form from using @clr/angular to @cds/angular and encountered a most likely unintended behaviour. When the user saves the form, by clicking on the button, after the successful save the corresponding loadingStatus should be set to indicate the loading behavior to the user. Additionally the form is no longer dirty, as it does not have any unsaved changes and thus the save button should not be enabled anymore. However settings the disabled state does not take any effect after the loadingState becomes "default" again.

How to reproduce

I've build a reproduction on Stackblitz using one of the provided templates. Thank you for providing the templates, this made reporting this behavior a lot easier!

Steps to reproduce the behavior:

  1. Open example on Stackblitz
  2. Click on the "Simulate loading ..." button.
  3. Observe the "... this does not." button being enabled after the loading, even though the disabledState changed to and is still true.

Expected behavior

The cds-button should base its disabled state solely on the corresponding attribute, once the loadingState is reverted back to default. Altering the value of disabled while the loadingState is something other than "default" should not be ignored.

Versions

Clarity project:

Clarity version:

Framework:

Framework version: Angular 13

Device:

Additional notes

Add any other notes about the problem here.

ashleyryan commented 2 years ago

This is interesting. We very specifically have logic that says "if the loading state switches to default from one of the others, enable the button": https://github.com/vmware-clarity/core/blob/0e2557b688332c43c304f4e01b2c92208394bc2b/projects/core/src/button/button.element.ts#L102

I'd have to think through the implications of changing this behavior. As it stands now, when the loadingState changes to loading or success or error, the button is disabled. That behavior is expected. But it seems that to you, the converse should not be true - when switching from one of those state back to default, the button shouldn't automatically be enabled.

What is prompting you to switch back from the success state to the default state instead of keeping the success state, if you want the button to still be disabled?

EndzeitBegins commented 2 years ago

From an outside perspective I would've assumed the component being displayed solely based on it's inputs; that is given the same input values for loadingState and disabled always yields the same result, regardeless of in which order those inputs were set or which values they had beforehand. Basically I assumed the component to be stateless / behave like a pure function, where all its inputs are taken into account.

disabled loadingState result
true default disabled
false default enabled
true non-default disabled
false non-default disabled

What is prompting you to switch back from the success state to the default state instead of keeping the success state, if you want the button to still be disabled?

That might be a workable solution for now. I'd only have to change back to "default" once the form is dirty again. Thanks for the hint. I'll propose this solution tomorrow.

Nonetheless I personally would find it more intuitive to work as described earlier. But maybe that's just me.

If you want to change this behaviour, I would be glad to help, even though I'm unsure if that's a feasible first contribution. In case you deem this to work as intended feel free to close this issue. I just stumbled upon this and thought of it to be unwanted.

In any case, thanks for the quick reply. And the great framework.

ashleyryan commented 2 years ago

Try that as a work around for now - you could also pretty quickly toggle back the disabled status after removing the success status.

I think it's reasonable to keep the state set by the dev, if they're controlling the state of the button themselves. We have some logic around that with the private _disabled property in the component. You can take a look there if you want to take a crack at it. It's probably not something I can get to immediately.

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 6.1.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 2 years ago

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.