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

fix(button): restore disabled state set from outside when loading sta… #130

Closed EndzeitBegins closed 2 years ago

EndzeitBegins commented 2 years ago

…te changes to default

Signed-off-by: EndzeitBegins 16666115+EndzeitBegins@users.noreply.github.com

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

What is the current behavior?

Issue Number: #129

What is the new behavior?

When changing to the loadingState to default the disabled value set from outside is restored, whether is was set before or while the loadingState was non-default.

Does this PR introduce a breaking change?

Other information

My solution includes replacing the field disabled in CdsBaseButton with a corresponding getter and setter in order to override its behavior in CdsButton. This in turn adds a private field _disabled to CdsBaseButton. To avoid collisions I renamed _disabled to _disabled_from_outside in CdsButton. According to npm run public-api:check this results in changes to the "public" API, even though only private fields were added / renamed and the field disabled replaced with getter / setter.

vmwclabot commented 2 years ago

@EndzeitBegins, you must sign our contributor license agreement before your changes are merged. Click here to sign the agreement. If you are a VMware employee, read this for further instruction.

ashleyryan commented 2 years ago

Thanks for taking a stab at this, I'm reviewing and looking through it now. I don't love the idea of completely disregarding the value attempting to be set in the setter.

We already had the _disabled hidden variable that was tracking the manually set disabled status. And uses it to set the disabled value when the loading state changes back to default. I'm trying to first understand why this isn't doing the job in Angular. Is it because Angular is bulk updating the web component properties at the same time (settings loadingState and disabled at the same time?) or in a different order than expected (setting loadingState before disabled)? That would help me better understand your fix.

Likewise, I'm not sure I follow your unit tests updates and what they're attempting to do that's different than what they did before.

vmwclabot commented 2 years ago

@EndzeitBegins, VMware has approved your signed contributor license agreement.

EndzeitBegins commented 2 years ago

Thank you for taking a look at my proposal @ashleyryan.

There were two tests which ensured that the disabled state (true / false), which is set while the loadingState is "default", thus before changing the loadingState to "loading", is restored, once the loadingState becomes "default" again. I just renamed them and set the disabled=false explicitly to highlight the difference between the two tests. This use case works.

However, I added two tests which reproduce the problem I originally encountered using the Angular integration. As I'm able to reproduce this without Angular being involved, I'm quite certain this is a bug in the core framework. What my test case shows, is that when changing the disabled state when the loadingState is non-default, e.g. "loading", the value is not restored once the loadingState is set back to "default" again.

I'm haven't worked with lit before but from what I was seeing this is due to the outside "change" in disabled not being registered, as disabled is true already due to the loadingState being "loading".

Also, in my proposal the disabled state is never completely disregarded. The opposite is the case, once the "default" loadingState is reached again, the disabled state set from outside is applied again - in contrast to the current implementation, which just ignores that disabled=true was being set while the loadingState was "loading".

Trying to summarize the difference:

// Assuming defaults
disabled = false;
loadingState = "default";

// Use case 1 - setting before loadingState / with default 
disabled = true;
loadingState = "loading";
loadingState = "default"; // return back to "default"
-> disabled is true (which is correct imo)

// Use case 2 - setting after loadingState / with non-default
loadingState = "loading";
disabled = true;
loadingState = "default"; // return back to "default", restores value before loadingState changed 
-> disabled is false (which is incorrect imo)
ashleyryan commented 2 years ago

This is causing some of the react snapshot/unit tests to fail and I'm investigating why.

EndzeitBegins commented 2 years ago

Thank you @ashleyryan for investing time looking into this issue. I'm not quite certain whether there is something I can help you with at this moment. Let me know if I can be of any help.

Should I change my pull-request in any way? Or do you simply take over the issue?

I've found a slightly different solution that also passes the added tests. But still fails the aforementioned snapshot tests, as lit seems to add the attribute disabled="true" instead of former disabled="" to the DOM. Sadly I don't know why that is the case or whether that is an actual problem or not. However it no longer needs the conditional logic in the setter as you've lined out.

The idea is to basically use the getter to determine the disabled state both on the value stored in the parent and the loadingState:

  get disabled(): boolean {
    return super.disabled || !this.isDefaultLoadingState(this.loadingState);
  }

  set disabled(value: boolean) {
    super.disabled = value;
  }

Coupled with notifying lit about the potential update whenever the loadingState changes, this seems to have the same effect, without requiring an additional private property or explicit setting of the disabled state.

  update(props: PropertyValues<this>) {
    if (props.has('loadingState')) {
      // change in loadingState might also affect disabled state
      this.requestUpdate('disabled', super.disabled);

      if (this.isDefaultLoadingState(this.loadingState)) {
        this.style.removeProperty('width');
      } else {
        this.style.width = getElementWidth(this);
      }
    }

    super.update(props);
  }

With this approach, the override of firstUpdated can be dropped altogether.

ashleyryan commented 2 years ago

I can either take the PR over or you can try and make some of the changes you or I have suggested.

Why don't you try updating your commit to have the alternative implementation, I like that approach. We also need to update the commit message to include a link to the original issue. So update it to something like

fix(button): restore disabled state when loading state finishes

fixes #130

I've spent most of the day digging into why lit is doing that with boolean attributes and I've been talking to them. I've narrowed it down to something not working correctly when using a getter or setter vs a simple property. I think it's fine and I can update the react tests if necessary. But let's try the updated solution for now.

I'm also happy to take it over, if you'd like me to.

ashleyryan commented 2 years ago

I ended up taking it over @EndzeitBegins . I couldn't get your suggested approach to work - even though the tests were passing I was finding that in storybook when I was manipulating the component with 

document.querySelector('cds-button').loadingState = 'loading';
document.querySelector('cds-button').loadingState = 'default';

That it wasn't losing the disabled status for some reason.

EndzeitBegins commented 2 years ago

Thanks for looking into this and finding a solution that works with the remaining use cases, @ashleyryan.

I hope my initial test cases were helpful in aiding the way at least. Looking forward to the merge!

github-actions[bot] commented 2 years ago

:tada: This PR is included 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 PRs after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.