From 4f8ccb486a85cb2581a7553f6ba9b5db6294218e Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Tue, 1 Nov 2016 13:20:17 -0700 Subject: [PATCH 1/3] fix(radio): only emit change event with user input --- src/lib/radio/radio.spec.ts | 21 ++++++++++++--------- src/lib/radio/radio.ts | 16 +++++++++++----- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/lib/radio/radio.spec.ts b/src/lib/radio/radio.spec.ts index 361854ddae93..5545a4fb55a7 100644 --- a/src/lib/radio/radio.spec.ts +++ b/src/lib/radio/radio.spec.ts @@ -152,21 +152,22 @@ describe('MdRadio', () => { expect(spies[1]).toHaveBeenCalledTimes(1); }); - it('should emit a change event from the radio group', () => { + it('should not emit a change event from the radio group when change group value ' + + 'programmatically', () => { expect(groupInstance.value).toBeFalsy(); let changeSpy = jasmine.createSpy('radio-group change listener'); groupInstance.change.subscribe(changeSpy); - groupInstance.value = 'fire'; + radioLabelElements[0].click(); fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalled(); + expect(changeSpy).toHaveBeenCalledTimes(1); groupInstance.value = 'water'; fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalledTimes(2); + expect(changeSpy).toHaveBeenCalledTimes(1); }); // TODO(jelbourn): test this in an e2e test with *real* focus, rather than faking @@ -242,34 +243,36 @@ describe('MdRadio', () => { fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalled(); + expect(changeSpy).toHaveBeenCalledTimes(0); expect(groupInstance.value).toBeTruthy(); radioInstances[0].checked = false; fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalledTimes(2); + expect(changeSpy).toHaveBeenCalledTimes(0); expect(groupInstance.value).toBeFalsy(); expect(radioInstances.every(radio => !radio.checked)).toBe(true); expect(groupInstance.selected).toBeNull(); }); - it('should fire a change event from the group whenever a radio checked state changes', () => { + it('should not fire a change event from the group when a radio checked state changes', () => { let changeSpy = jasmine.createSpy('radio-group change listener'); groupInstance.change.subscribe(changeSpy); radioInstances[0].checked = true; fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalled(); + expect(changeSpy).toHaveBeenCalledTimes(0); expect(groupInstance.value).toBeTruthy(); + expect(groupInstance.value).toBe('fire'); radioInstances[1].checked = true; fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalledTimes(2); + expect(groupInstance.value).toBe('water'); + expect(changeSpy).toHaveBeenCalledTimes(0); }); }); diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts index f272a8a82a2b..bae13f07c039 100644 --- a/src/lib/radio/radio.ts +++ b/src/lib/radio/radio.ts @@ -127,11 +127,6 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { this._value = newValue; this._updateSelectedRadioFromValue(); - - // Only fire a change event if this isn't the first time the value is ever set. - if (this._isInitialized) { - this._emitChangeEvent(); - } } } @@ -171,6 +166,13 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { } } + emitChangeEvent(): void { + // Only fire a change event if this isn't the first time the value is ever set. + if (this._isInitialized) { + this._emitChangeEvent(); + } + } + private _updateRadioButtonNames(): void { if (this._radios) { this._radios.forEach(radio => { @@ -418,12 +420,16 @@ export class MdRadioButton implements OnInit { // emit its event object to the `change` output. event.stopPropagation(); + let groupValueChanged = this.value != this.radioGroup.value; this.checked = true; this._emitChangeEvent(); if (this.radioGroup) { this.radioGroup._controlValueAccessorChangeFn(this.value); this.radioGroup._touch(); + if (groupValueChanged) { + this.radioGroup.emitChangeEvent(); + } } } From 7c7cff7ec11025a4d739ff81b83d20675a04732c Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Tue, 1 Nov 2016 14:00:46 -0700 Subject: [PATCH 2/3] Fix the error when there's no radioGroup --- src/lib/radio/radio.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts index bae13f07c039..bd71acd4bbb1 100644 --- a/src/lib/radio/radio.ts +++ b/src/lib/radio/radio.ts @@ -420,7 +420,7 @@ export class MdRadioButton implements OnInit { // emit its event object to the `change` output. event.stopPropagation(); - let groupValueChanged = this.value != this.radioGroup.value; + let groupValueChanged = this.radioGroup && this.value != this.radioGroup.value; this.checked = true; this._emitChangeEvent(); From f360ffe0bdb1383c545ad290e83eefaab6150fab Mon Sep 17 00:00:00 2001 From: Yuan Gao Date: Tue, 1 Nov 2016 14:10:57 -0700 Subject: [PATCH 3/3] Addressed comments --- src/lib/radio/radio.spec.ts | 16 ++++++++-------- src/lib/radio/radio.ts | 21 ++++++++------------- 2 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/lib/radio/radio.spec.ts b/src/lib/radio/radio.spec.ts index 5545a4fb55a7..6fa1eb6642fe 100644 --- a/src/lib/radio/radio.spec.ts +++ b/src/lib/radio/radio.spec.ts @@ -152,8 +152,8 @@ describe('MdRadio', () => { expect(spies[1]).toHaveBeenCalledTimes(1); }); - it('should not emit a change event from the radio group when change group value ' - + 'programmatically', () => { + it(`should not emit a change event from the radio group when change group value + programmatically`, () => { expect(groupInstance.value).toBeFalsy(); let changeSpy = jasmine.createSpy('radio-group change listener'); @@ -235,22 +235,22 @@ describe('MdRadio', () => { } })); - it('should update the group\'s selected radio to null when unchecking that radio ' - + 'programmatically', () => { + it(`should update the group's selected radio to null when unchecking that radio + programmatically`, () => { let changeSpy = jasmine.createSpy('radio-group change listener'); groupInstance.change.subscribe(changeSpy); radioInstances[0].checked = true; fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalledTimes(0); + expect(changeSpy).not.toHaveBeenCalled(); expect(groupInstance.value).toBeTruthy(); radioInstances[0].checked = false; fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalledTimes(0); + expect(changeSpy).not.toHaveBeenCalled(); expect(groupInstance.value).toBeFalsy(); expect(radioInstances.every(radio => !radio.checked)).toBe(true); expect(groupInstance.selected).toBeNull(); @@ -263,7 +263,7 @@ describe('MdRadio', () => { fixture.detectChanges(); - expect(changeSpy).toHaveBeenCalledTimes(0); + expect(changeSpy).not.toHaveBeenCalled(); expect(groupInstance.value).toBeTruthy(); expect(groupInstance.value).toBe('fire'); @@ -272,7 +272,7 @@ describe('MdRadio', () => { fixture.detectChanges(); expect(groupInstance.value).toBe('water'); - expect(changeSpy).toHaveBeenCalledTimes(0); + expect(changeSpy).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/radio/radio.ts b/src/lib/radio/radio.ts index bd71acd4bbb1..ff922ca58468 100644 --- a/src/lib/radio/radio.ts +++ b/src/lib/radio/radio.ts @@ -166,13 +166,6 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { } } - emitChangeEvent(): void { - // Only fire a change event if this isn't the first time the value is ever set. - if (this._isInitialized) { - this._emitChangeEvent(); - } - } - private _updateRadioButtonNames(): void { if (this._radios) { this._radios.forEach(radio => { @@ -199,11 +192,13 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor { } /** Dispatch change event with current selection and group value. */ - private _emitChangeEvent(): void { - let event = new MdRadioChange(); - event.source = this._selected; - event.value = this._value; - this.change.emit(event); + _emitChangeEvent(): void { + if (this._isInitialized) { + let event = new MdRadioChange(); + event.source = this._selected; + event.value = this._value; + this.change.emit(event); + } } /** @@ -428,7 +423,7 @@ export class MdRadioButton implements OnInit { this.radioGroup._controlValueAccessorChangeFn(this.value); this.radioGroup._touch(); if (groupValueChanged) { - this.radioGroup.emitChangeEvent(); + this.radioGroup._emitChangeEvent(); } } }