fix(button): add disable button behavior #76

chrsmrtn- · 2017-06-05T20:10:28Z

this adds in disabled behavior when defined for <ux-button></ux-button>

resolves #75

CLAassistant · 2017-06-05T20:10:37Z

All committers have signed the CLA.

RichiCoder1 · 2017-06-06T18:52:22Z

src/button/ux-button.ts

@@ -12,7 +12,7 @@ export class UxButton implements Themable {
  @bindable public type = null;
  @bindable public size = null;
  @bindable public effect = null;
-  @bindable public disabled = false;
+  @bindable public disabled: any = false;


Couldn't this be @bindable public disabled: boolean | string = false?

@ZHollingshead Let's definitely use the typings mentinoed by @RichiCoder1

I like this a lot.

RichiCoder1 · 2017-06-06T18:55:22Z

src/button/ux-button.ts

@@ -29,12 +29,24 @@ export class UxButton implements Themable {
    if (this.theme) {
      this.styleEngine.applyTheme(this, this.theme);
    }
+
+    if (this.disabled || this.disabled === '') {


I'm curious what the reasoning is behind this line? It seems contradictory.

I think it's to treat empty strings as true? It is confusing. @ZHollingshead If we switch to the more explicit typing above we can probably write some clearer logic here as well.

We should be able to check directly against false or empty with stricter types.

@ZHollingshead, hey wanted to run this possible change past ya and get feedback. I've casted the disabled variable per @RichiCoder1's suggestion:

@bindable public disabled: boolean | string = false;

Now I'm working on the subsequent if block to clear that logic up a bit. Specifically, I wanted to highlight that we are attempting to detect an empty string as true. All other casting from string to boolean will behave as normal. As well as all other castings to boolean will behave as normal.

// ensure we cast empty string as true if (typeof this.disabled === 'string' && this.disabled === '') { this.disabled = true; } if (this.disabled) { this.button.setAttribute('disabled', ''); }

Also wondering if we should be using this.button.setAttribute('disabled', 'true'); instead like we've done in the disabledChanged hander?

That logic makes much more sense now. Is there a meaningful difference in browser between disabled="" and disabled="true"? If not, I'd opt for the former in both places.

Here's what I've found. It seems that disabled="true" will result to false. This stackoverflow answer references the HTML 5 spec:

https://stackoverflow.com/questions/6961526/correct-value-for-disabled-attribute

I'm almost thinking now that the original intention of that snip of code was to follow the HTML 5 spec. I took my original code from the ux-input.ts source to stay consistent within the project.

@EisenbergEffect @ZHollingshead, does the above sound right when ux-input was written out?

After we finalize this behavior and coding for the ux-button, i'll make another pull request to move these changes into other components that use the disabled and readonly attributes.

Ahh, yes, that's the correct SO I was looking for. Boolean attributes in HTML are a bit weird about that. I'd def do setAttibute(attributeName, '') or all instances of disabled and readonly. I don't think Aurelia is too concerned with XHTML compatibility.

@chrsmrtn- setAttribute('disabled', '') is the way to go in my opinion.

EisenbergEffect · 2017-06-06T19:56:53Z

src/button/ux-button.ts

@@ -12,7 +12,7 @@ export class UxButton implements Themable {
  @bindable public type = null;
  @bindable public size = null;
  @bindable public effect = null;
-  @bindable public disabled = false;
+  @bindable public disabled: any = false;


@ZHollingshead Let's definitely use the typings mentinoed by @RichiCoder1

EisenbergEffect · 2017-06-06T19:57:33Z

src/button/ux-button.ts

@@ -29,12 +29,24 @@ export class UxButton implements Themable {
    if (this.theme) {
      this.styleEngine.applyTheme(this, this.theme);
    }
+
+    if (this.disabled || this.disabled === '') {


I think it's to treat empty strings as true? It is confusing. @ZHollingshead If we switch to the more explicit typing above we can probably write some clearer logic here as well.

serifine · 2017-06-06T21:14:55Z

src/button/ux-button.ts

@@ -12,7 +12,7 @@ export class UxButton implements Themable {
  @bindable public type = null;
  @bindable public size = null;
  @bindable public effect = null;
-  @bindable public disabled = false;
+  @bindable public disabled: any = false;


Let's go ahead and change this as mentioned above.

serifine · 2017-06-06T21:16:23Z

src/button/ux-button.ts

@@ -29,12 +29,24 @@ export class UxButton implements Themable {
    if (this.theme) {
      this.styleEngine.applyTheme(this, this.theme);
    }
+
+    if (this.disabled || this.disabled === '') {


We should be able to check directly against false or empty with stricter types.

chrsmrtn- · 2017-06-06T21:52:46Z

src/button/ux-button.ts

  }

  public themeChanged(newValue: any) {
    this.styleEngine.applyTheme(this, newValue);
  }

+  public disabledChanged(newValue: any) {


Should we also adjust this casting to match what we've done for the disabled attribute? So this would now become:
public disabledChanged(newValue: boolean | string) {?

Yup! Though in this case it'd be boolean | string | null as I believe an end user could technically null it out.

This appears to be called by the binding stuff within Aurelia. I'm going to move this to boolean | string for now and align the if statement to match for clarity. If something else comes later on down the road, we can re-visit this bit.

RichiCoder1 · 2017-06-08T03:25:33Z

src/button/ux-button.ts

  }

  public themeChanged(newValue: any) {
    this.styleEngine.applyTheme(this, newValue);
  }

+  public disabledChanged(newValue: boolean | string) {
+    // ensure we cast empty string as true
+    if (typeof this.disabled === 'string' && this.disabled === '') {


Did you mean if (typeof newValue === 'string' && newValue = '') {?

Yep that I did!

will fix to:

if (typeof newValue === 'string' && newValue === '') {

Funny thing is, in my testing that if block was never hit as the binding code is sending boolean values for the update task all the time.

bug(button): add disable button behavior

22e6176

serifine changed the title ~~bug(button): add disable button behavior~~ fix(button): add disable button behavior Jun 6, 2017

serifine requested a review from EisenbergEffect June 6, 2017 11:22

serifine approved these changes Jun 6, 2017

View reviewed changes

RichiCoder1 reviewed Jun 6, 2017

View reviewed changes

EisenbergEffect suggested changes Jun 6, 2017

View reviewed changes

serifine suggested changes Jun 6, 2017

View reviewed changes

chrsmrtn- commented Jun 6, 2017

View reviewed changes

chrsmrtn- added 2 commits June 6, 2017 16:49

fix(button): add disabled button behavior css styling for raised and fab

752d261

fix(button) adjusted fix for casting and clarity

b04fb3c

RichiCoder1 suggested changes Jun 8, 2017

View reviewed changes

fix(button): fix bad ref in disabledChanged handler

faa369f

RichiCoder1 approved these changes Jun 8, 2017

View reviewed changes

serifine approved these changes Jun 8, 2017

View reviewed changes

serifine merged commit a16f47f into aurelia:master Jun 9, 2017

chrsmrtn- deleted the ux-button-disabled-click-issue branch June 9, 2017 15:08

chrsmrtn- mentioned this pull request Aug 2, 2017

refactor(components): disabled and readonly properties #88

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(button): add disable button behavior #76

fix(button): add disable button behavior #76

chrsmrtn- commented Jun 5, 2017

CLAassistant commented Jun 5, 2017 •

edited

Loading

RichiCoder1 Jun 6, 2017

EisenbergEffect Jun 6, 2017

serifine Jun 6, 2017

RichiCoder1 Jun 6, 2017

EisenbergEffect Jun 6, 2017

serifine Jun 6, 2017

chrsmrtn- Jun 6, 2017

RichiCoder1 Jun 6, 2017

chrsmrtn- Jun 6, 2017 •

edited

Loading

RichiCoder1 Jun 6, 2017

serifine Jun 7, 2017

EisenbergEffect Jun 6, 2017

EisenbergEffect Jun 6, 2017

serifine Jun 6, 2017

serifine Jun 6, 2017

chrsmrtn- Jun 6, 2017 •

edited

Loading

RichiCoder1 Jun 6, 2017

chrsmrtn- Jun 7, 2017 •

edited

Loading

RichiCoder1 Jun 8, 2017

chrsmrtn- Jun 8, 2017 •

edited

Loading

fix(button): add disable button behavior #76

fix(button): add disable button behavior #76

Conversation

chrsmrtn- commented Jun 5, 2017

CLAassistant commented Jun 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrsmrtn- Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrsmrtn- Jun 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrsmrtn- Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrsmrtn- Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

CLAassistant commented Jun 5, 2017 •

edited

Loading

chrsmrtn- Jun 6, 2017 •

edited

Loading

chrsmrtn- Jun 6, 2017 •

edited

Loading

chrsmrtn- Jun 7, 2017 •

edited

Loading

chrsmrtn- Jun 8, 2017 •

edited

Loading