fix(radio): Make MdRadioButton change detection strategy OnPush (#2526)

* Make MdRadioButton OnPush * . * Fixed tests * fixed group ripple disabled test * fix lint * fix test * sync and fix test * address comments * address comments
angular · May 25, 2017 · 97a9bdc · 97a9bdc
1 parent 57c45a1
commit 97a9bdc
Show file tree

Hide file tree

Showing 2 changed files with 70 additions and 11 deletions.
diff --git a/src/lib/radio/radio.spec.ts b/src/lib/radio/radio.spec.ts
@@ -229,7 +229,7 @@ describe('MdRadio', () => {
     });
 
     it('should not show ripples on disabled radio buttons', () => {
-      radioInstances[0].disabled = true;
+      testComponent.isFirstDisabled = true;
       fixture.detectChanges();
 
       dispatchFakeEvent(radioLabelElements[0], 'mousedown');
@@ -238,7 +238,7 @@ describe('MdRadio', () => {
       expect(radioNativeElements[0].querySelectorAll('.mat-ripple-element').length)
         .toBe(0, 'Expected a disabled radio button to not show ripples');
 
-      radioInstances[0].disabled = false;
+      testComponent.isFirstDisabled = false;
       fixture.detectChanges();
 
       dispatchFakeEvent(radioLabelElements[0], 'mousedown');
@@ -417,11 +417,13 @@ describe('MdRadio', () => {
 
     it('should write to the radio button based on ngModel', fakeAsync(() => {
       testComponent.modelValue = 'chocolate';
+
       fixture.detectChanges();
       tick();
       fixture.detectChanges();
 
       expect(innerRadios[1].nativeElement.checked).toBe(true);
+      expect(radioInstances[1].checked).toBe(true);
     }));
 
     it('should update the ngModel value when selecting a radio button', () => {
@@ -551,7 +553,7 @@ describe('MdRadio', () => {
     it('should change aria-label attribute if property is changed at runtime', () => {
       expect(fruitRadioNativeInputs[0].getAttribute('aria-label')).toBe('Banana');
 
-      fruitRadioInstances[0].ariaLabel = 'Pineapple';
+      testComponent.ariaLabel = 'Pineapple';
       fixture.detectChanges();
 
       expect(fruitRadioNativeInputs[0].getAttribute('aria-label')).toBe('Pineapple');
@@ -568,7 +570,7 @@ describe('MdRadio', () => {
     it('should change aria-labelledby attribute if property is changed at runtime', () => {
       expect(fruitRadioNativeInputs[0].getAttribute('aria-labelledby')).toBe('xyz');
 
-      fruitRadioInstances[0].ariaLabelledby = 'uvw';
+      testComponent.ariaLabelledby = 'uvw';
       fixture.detectChanges();
 
       expect(fruitRadioNativeInputs[0].getAttribute('aria-labelledby')).toBe('uvw');
@@ -593,7 +595,8 @@ describe('MdRadio', () => {
                   [labelPosition]="labelPos"
                   [value]="groupValue"
                   name="test-name">
-    <md-radio-button value="fire" [disableRipple]="disableRipple">Charmander</md-radio-button>
+    <md-radio-button value="fire" [disableRipple]="disableRipple"
+                     [disabled]="isFirstDisabled">Charmander</md-radio-button>
     <md-radio-button value="water" [disableRipple]="disableRipple">Squirtle</md-radio-button>
     <md-radio-button value="leaf" [disableRipple]="disableRipple">Bulbasaur</md-radio-button>
   </md-radio-group>
@@ -602,6 +605,7 @@ describe('MdRadio', () => {
 class RadiosInsideRadioGroup {
   labelPos: 'before' | 'after';
   isGroupDisabled: boolean = false;
+  isFirstDisabled: boolean = false;
   groupValue: string = null;
   disableRipple: boolean = false;
 }
@@ -618,12 +622,18 @@ class RadiosInsideRadioGroup {
     <md-radio-button name="weather" value="cool">Autumn</md-radio-button>
 
     <span id="xyz">Baby Banana</span>
-    <md-radio-button name="fruit" value="banana" aria-label="Banana" aria-labelledby="xyz">
+    <md-radio-button name="fruit"
+                     value="banana"
+                     [aria-label]="ariaLabel"
+                     [aria-labelledby]="ariaLabelledby">
     </md-radio-button>
     <md-radio-button name="fruit" value="raspberry">Raspberry</md-radio-button>
   `
 })
-class StandaloneRadioButtons { }
+class StandaloneRadioButtons {
+  ariaLabel: string = 'Banana';
+  ariaLabelledby: string = 'xyz';
+}
 
 
 @Component({

diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts
@@ -1,5 +1,7 @@
 import {
   AfterContentInit,
+  ChangeDetectionStrategy,
+  ChangeDetectorRef,
   Component,
   ContentChildren,
   Directive,
@@ -86,6 +88,12 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
   /** Whether the `value` has been set to its initial value. */
   private _isInitialized: boolean = false;
 
+  /** Whether the labels should appear after or before the radio-buttons. Defaults to 'after' */
+  private _labelPosition: 'before' | 'after' = 'after';
+
+  /** Whether the radio group is disabled. */
+  private _disabled: boolean = false;
+
   /** The method to be called in order to update ngModel */
   _controlValueAccessorChangeFn: (value: any) => void = (value) => {};
 
@@ -129,8 +137,17 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
     this.labelPosition = (v == 'start') ? 'after' : 'before';
   }
 
+
   /** Whether the labels should appear after or before the radio-buttons. Defaults to 'after' */
-  @Input() labelPosition: 'before' | 'after' = 'after';
+  @Input()
+  get labelPosition() {
+    return this._labelPosition;
+  }
+
+  set labelPosition(v) {
+    this._labelPosition = (v == 'before') ? 'before' : 'after';
+    this._markRadiosForCheck();
+  }
 
   /** Value of the radio button. */
   @Input()
@@ -160,6 +177,18 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
     this._checkSelectedRadioButton();
   }
 
+  /** Whether the radio group is diabled */
+  @Input()
+  get disabled() { return this._disabled; }
+  set disabled(value) {
+    this._disabled = value;
+    this._markRadiosForCheck();
+  }
+
+  constructor(private _changeDetector: ChangeDetectorRef) {
+    super();
+  }
+
   /**
    * Initialize properties once content children are available.
    * This allows us to propagate relevant attributes to associated buttons.
@@ -215,12 +244,19 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
     }
   }
 
+  _markRadiosForCheck() {
+    if (this._radios) {
+      this._radios.forEach(radio => radio._markForCheck());
+    }
+  }
+
   /**
    * Sets the model value. Implemented as part of ControlValueAccessor.
    * @param value
    */
   writeValue(value: any) {
     this.value = value;
+    this._changeDetector.markForCheck();
   }
 
   /**
@@ -247,6 +283,7 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
    */
   setDisabledState(isDisabled: boolean) {
     this.disabled = isDisabled;
+    this._changeDetector.markForCheck();
   }
 }
 
@@ -264,7 +301,8 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
     '[class.mat-radio-checked]': 'checked',
     '[class.mat-radio-disabled]': 'disabled',
     '[attr.id]': 'id',
-  }
+  },
+  changeDetection: ChangeDetectionStrategy.OnPush,
 })
 export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
 
@@ -307,6 +345,7 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
         // Notify all radio buttons with the same name to un-check.
         this._radioDispatcher.notify(this.id, this.name);
       }
+      this._changeDetector.markForCheck();
     }
   }
 
@@ -328,7 +367,6 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
           this.radioGroup.selected = this;
         }
       }
-
     }
   }
 
@@ -364,7 +402,6 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
   get disabled(): boolean {
     return this._disabled || (this.radioGroup != null && this.radioGroup.disabled);
   }
-
   set disabled(value: boolean) {
     this._disabled = coerceBooleanProperty(value);
   }
@@ -408,6 +445,7 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
   constructor(@Optional() radioGroup: MdRadioGroup,
               private _elementRef: ElementRef,
               private _renderer: Renderer2,
+              private _changeDetector: ChangeDetectorRef,
               private _focusOriginMonitor: FocusOriginMonitor,
               private _radioDispatcher: UniqueSelectionDispatcher) {
     // Assertions. Ideally these should be stripped out by the compiler.
@@ -427,6 +465,17 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
     this._focusOriginMonitor.focusVia(this._inputElement.nativeElement, this._renderer, 'keyboard');
   }
 
+  /**
+   * Marks the radio button as needing checking for change detection.
+   * This method is exposed because the parent radio group will directly
+   * update bound properties of the radio button.
+   */
+  _markForCheck() {
+    // When group value changes, the button will not be notified. Use `markForCheck` to explicit
+    // update radio button's status
+    this._changeDetector.markForCheck();
+  }
+
   ngOnInit() {
     if (this.radioGroup) {
       // If the radio is inside a radio group, determine if it should be checked