From a8d13dcc45f581b22f5c57d478bc9b73f5f30276 Mon Sep 17 00:00:00 2001 From: Robert Messerle Date: Thu, 19 May 2016 14:43:42 -0700 Subject: [PATCH 1/2] fix(radio): resolves change detection issues Using *ngFor to iterate over options was causing the error "Expression has changed after it was checked. Previous value: undefined." closes #403 --- src/components/radio/radio.spec.ts | 48 ++++++++++++++++++------------ src/components/radio/radio.ts | 34 +++++++++++---------- 2 files changed, 48 insertions(+), 34 deletions(-) diff --git a/src/components/radio/radio.spec.ts b/src/components/radio/radio.spec.ts index 3d4333ef6256..da1867697d1a 100644 --- a/src/components/radio/radio.spec.ts +++ b/src/components/radio/radio.spec.ts @@ -175,27 +175,30 @@ describe('MdRadio', () => { expect(radioNativeElements[0].classList).not.toContain('md-radio-focused'); }); - it('should update the group and radios when updating the group value', () => { + it('should update the group and radios when updating the group value', async(() => { expect(groupInstance.value).toBeFalsy(); testComponent.groupValue = 'fire'; fixture.detectChanges(); - expect(groupInstance.value).toBe('fire'); - expect(groupInstance.selected).toBe(radioInstances[0]); - expect(radioInstances[0].checked).toBe(true); - expect(radioInstances[1].checked).toBe(false); + fixture.whenStable().then(() => { + expect(groupInstance.value).toBe('fire'); + expect(groupInstance.selected).toBe(radioInstances[0]); + expect(radioInstances[0].checked).toBe(true); + expect(radioInstances[1].checked).toBe(false); - testComponent.groupValue = 'water'; - fixture.detectChanges(); - - expect(groupInstance.value).toBe('water'); - expect(groupInstance.selected).toBe(radioInstances[1]); - expect(radioInstances[0].checked).toBe(false); - expect(radioInstances[1].checked).toBe(true); - }); + testComponent.groupValue = 'water'; + fixture.detectChanges(); + return fixture.whenStable(); + }).then(() => { + expect(groupInstance.value).toBe('water'); + expect(groupInstance.selected).toBe(radioInstances[1]); + expect(radioInstances[0].checked).toBe(false); + expect(radioInstances[1].checked).toBe(true); + }); + })); - it('should deselect all of the checkboxes when the group value is cleared', () => { + it('should deselect all of the checkboxes when the group value is cleared', fakeAsync(() => { radioInstances[0].checked = true; fixture.detectChanges(); @@ -203,8 +206,10 @@ describe('MdRadio', () => { groupInstance.value = null; fixture.detectChanges(); + tick(); + expect(radioInstances.every(radio => !radio.checked)).toBe(true); - }); + })); }); describe('group with ngModel', () => { @@ -365,21 +370,26 @@ class StandaloneRadioButtons { } directives: [MD_RADIO_DIRECTIVES, FORM_DIRECTIVES], template: ` - Vanilla - Chocolate - Strawberry + + {{option.label}} + ` }) class RadioGroupWithNgModel { modelValue: string; + options = [ + {label: 'Vanilla', value: 'vanilla'}, + {label: 'Chocolate', value: 'chocolate'}, + {label: 'Strawberry', value: 'strawberry'}, + ]; } // TODO(jelbourn): remove eveything below when Angular supports faking events. /** - * Dispatches a focus change event from an element. + * Dispatches a focus change event from an element. * @param eventName Name of the event, either 'focus' or 'blur'. * @param element The element from which the event will be dispatched. */ diff --git a/src/components/radio/radio.ts b/src/components/radio/radio.ts index ec881ebdd938..3fdcce7ac401 100644 --- a/src/components/radio/radio.ts +++ b/src/components/radio/radio.ts @@ -69,7 +69,7 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { private _value: any = null; /** The HTML name attribute applied to radio buttons in this group. */ - private _name: string = null; + private _name: string = `md-radio-group-${_uniqueIdCounter++}`; /** Disables all individual radio buttons assigned to this group. */ private _disabled: boolean = false; @@ -160,10 +160,6 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { * @internal */ ngAfterContentInit() { - if (!this._name) { - this.name = `md-radio-group-${_uniqueIdCounter++}`; - } - // Mark this component as initialized in AfterContentInit because the initial value can // possibly be set by NgModel on MdRadioGroup, and it is possible that the OnInit of the // NgModel occurs *after* the OnInit of the MdRadioGroup. @@ -183,7 +179,7 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { /** Updates the `selected` radio button from the internal _value state. */ private _updateSelectedRadioFromValue(): void { - // If the value already matches the selected radio, no dothing. + // If the value already matches the selected radio, do nothing. let isAlreadySelected = this._selected != null && this._selected.value == this._value; if (this._radios != null && !isAlreadySelected) { @@ -192,8 +188,8 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { if (matchingRadio) { this.selected = matchingRadio; } else if (this.value == null) { - this.selected = null; - this._radios.forEach(radio => { radio.checked = false; }); + this.selected = null; + this._radios.forEach(radio => { radio.checked = false; }); } } } @@ -206,7 +202,7 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { this.change.emit(event); } - /** + /** * Implemented as part of ControlValueAccessor. * @internal */ @@ -258,7 +254,7 @@ export class MdRadioButton implements OnInit { /** The unique ID for the radio button. */ @HostBinding('id') @Input() - id: string; + id: string = `md-radio-${_uniqueIdCounter++}`; /** Analog to HTML 'name' attribute used to group radios for unique selection. */ @Input() @@ -283,12 +279,25 @@ export class MdRadioButton implements OnInit { this.radioGroup = radioGroup; + if (this.radioGroup) { + this._listenToRadioGroup(); + } + radioDispatcher.listen((id: string, name: string) => { if (id != this.id && name == this.name) { this.checked = false; } }); } + private _listenToRadioGroup() { + // name from `md-radio-group` overrides button name + this.name = this.radioGroup.name; + + // listen to value changes on parent + this.radioGroup.change.subscribe((change: MdRadioChange) => { + this._checked = change.value == this.value; + }); + } get inputId(): string { return `${this.id}-input`; @@ -345,11 +354,6 @@ export class MdRadioButton implements OnInit { /** @internal */ ngOnInit() { - // All radio buttons must have a unique id. - if (!this.id) { - this.id = `md-radio-${_uniqueIdCounter++}`; - } - // If the radio is inside of a radio group and it matches that group's value upon // initialization, start off as checked. if (this.radioGroup && this.radioGroup.value === this._value) { From 2f009389faee326076b7b1b49826658b9e5c1d88 Mon Sep 17 00:00:00 2001 From: Robert Messerle Date: Thu, 19 May 2016 15:00:07 -0700 Subject: [PATCH 2/2] fix(radio): name property should be inherited from radio group --- src/components/radio/radio.spec.ts | 63 +++++++++++++++++++----------- src/components/radio/radio.ts | 39 ++++++------------ 2 files changed, 54 insertions(+), 48 deletions(-) diff --git a/src/components/radio/radio.spec.ts b/src/components/radio/radio.spec.ts index da1867697d1a..897a3ae789e6 100644 --- a/src/components/radio/radio.spec.ts +++ b/src/components/radio/radio.spec.ts @@ -175,41 +175,35 @@ describe('MdRadio', () => { expect(radioNativeElements[0].classList).not.toContain('md-radio-focused'); }); - it('should update the group and radios when updating the group value', async(() => { + it('should update the group and radios when updating the group value', () => { expect(groupInstance.value).toBeFalsy(); testComponent.groupValue = 'fire'; fixture.detectChanges(); - fixture.whenStable().then(() => { - expect(groupInstance.value).toBe('fire'); - expect(groupInstance.selected).toBe(radioInstances[0]); - expect(radioInstances[0].checked).toBe(true); - expect(radioInstances[1].checked).toBe(false); + expect(groupInstance.value).toBe('fire'); + expect(groupInstance.selected).toBe(radioInstances[0]); + expect(radioInstances[0].checked).toBe(true); + expect(radioInstances[1].checked).toBe(false); - testComponent.groupValue = 'water'; - fixture.detectChanges(); - return fixture.whenStable(); - }).then(() => { - expect(groupInstance.value).toBe('water'); - expect(groupInstance.selected).toBe(radioInstances[1]); - expect(radioInstances[0].checked).toBe(false); - expect(radioInstances[1].checked).toBe(true); - }); - })); + testComponent.groupValue = 'water'; + fixture.detectChanges(); - it('should deselect all of the checkboxes when the group value is cleared', fakeAsync(() => { + expect(groupInstance.value).toBe('water'); + expect(groupInstance.selected).toBe(radioInstances[1]); + expect(radioInstances[0].checked).toBe(false); + expect(radioInstances[1].checked).toBe(true); + }); + + it('should deselect all of the checkboxes when the group value is cleared', () => { radioInstances[0].checked = true; - fixture.detectChanges(); expect(groupInstance.value).toBeTruthy(); groupInstance.value = null; - fixture.detectChanges(); - tick(); expect(radioInstances.every(radio => !radio.checked)).toBe(true); - })); + }); }); describe('group with ngModel', () => { @@ -241,6 +235,31 @@ describe('MdRadio', () => { }); })); + it('should set individual radio names based on the group name', () => { + expect(groupInstance.name).toBeTruthy(); + for (let radio of radioInstances) { + expect(radio.name).toBe(groupInstance.name); + } + + groupInstance.name = 'new name'; + for (let radio of radioInstances) { + expect(radio.name).toBe(groupInstance.name); + } + }); + + it('should check the corresponding radio button on group value change', () => { + expect(groupInstance.value).toBeFalsy(); + for (let radio of radioInstances) { + expect(radio.checked).toBeFalsy(); + } + + groupInstance.value = 'vanilla'; + for (let radio of radioInstances) { + expect(radio.checked).toBe(groupInstance.value === radio.value); + } + expect(groupInstance.selected.value).toBe(groupInstance.value); + }); + it('should have the correct ngControl state initially and after interaction', fakeAsync(() => { // The control should start off valid, pristine, and untouched. expect(groupNgControl.valid).toBe(true); @@ -338,7 +357,7 @@ describe('MdRadio', () => { @Component({ directives: [MD_RADIO_DIRECTIVES], template: ` - + Charmander Squirtle Bulbasaur diff --git a/src/components/radio/radio.ts b/src/components/radio/radio.ts index 3fdcce7ac401..fa8b496b5d12 100644 --- a/src/components/radio/radio.ts +++ b/src/components/radio/radio.ts @@ -101,14 +101,7 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { set name(value: string) { this._name = value; - this._updateChildRadioNames(); - } - - /** Propagate name attribute to radio buttons. */ - private _updateChildRadioNames(): void { - (this._radios || []).forEach(radio => { - radio.name = this._name; - }); + this._updateRadioButtonNames(); } @Input() @@ -177,6 +170,12 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { } } + private _updateRadioButtonNames(): void { + (this._radios || []).forEach(radio => { + radio.name = this.name; + }); + } + /** Updates the `selected` radio button from the internal _value state. */ private _updateSelectedRadioFromValue(): void { // If the value already matches the selected radio, do nothing. @@ -202,7 +201,7 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { this.change.emit(event); } - /** + /** * Implemented as part of ControlValueAccessor. * @internal */ @@ -279,25 +278,12 @@ export class MdRadioButton implements OnInit { this.radioGroup = radioGroup; - if (this.radioGroup) { - this._listenToRadioGroup(); - } - radioDispatcher.listen((id: string, name: string) => { if (id != this.id && name == this.name) { this.checked = false; } }); } - private _listenToRadioGroup() { - // name from `md-radio-group` overrides button name - this.name = this.radioGroup.name; - - // listen to value changes on parent - this.radioGroup.change.subscribe((change: MdRadioChange) => { - this._checked = change.value == this.value; - }); - } get inputId(): string { return `${this.id}-input`; @@ -354,10 +340,11 @@ export class MdRadioButton implements OnInit { /** @internal */ ngOnInit() { - // If the radio is inside of a radio group and it matches that group's value upon - // initialization, start off as checked. - if (this.radioGroup && this.radioGroup.value === this._value) { - this.checked = true; + if (this.radioGroup) { + // If the radio is inside a radio group, determine if it should be checked + this.checked = this.radioGroup.value === this._value; + // Copy name from parent radio group + this.name = this.radioGroup.name; } }