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

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

Merged
merged 9 commits into from
May 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions src/lib/radio/radio.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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');
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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');
Expand All @@ -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');
Expand All @@ -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>
Expand All @@ -602,6 +605,7 @@ describe('MdRadio', () => {
class RadiosInsideRadioGroup {
labelPos: 'before' | 'after';
isGroupDisabled: boolean = false;
isFirstDisabled: boolean = false;
groupValue: string = null;
disableRipple: boolean = false;
}
Expand All @@ -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({
Expand Down
57 changes: 53 additions & 4 deletions src/lib/radio/radio.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import {
AfterContentInit,
ChangeDetectionStrategy,
ChangeDetectorRef,
Component,
ContentChildren,
Directive,
Expand Down Expand Up @@ -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) => {};

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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();
}

/**
Expand All @@ -247,6 +283,7 @@ export class MdRadioGroup extends _MdRadioGroupMixinBase
*/
setDisabledState(isDisabled: boolean) {
this.disabled = isDisabled;
this._changeDetector.markForCheck();
}
}

Expand All @@ -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 {

Expand Down Expand Up @@ -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();
}
}

Expand All @@ -328,7 +367,6 @@ export class MdRadioButton implements OnInit, AfterViewInit, OnDestroy {
this.radioGroup.selected = this;
}
}

}
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down