Skip to content

Commit

Permalink
Fix/403/radio without name (#478)
Browse files Browse the repository at this point in the history 
* 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

* fix(radio): name property should be inherited from radio group
  • Loading branch information
@robertmesserle @jelbourn
robertmesserle authored and jelbourn committed May 25, 2016
1 parent fcc4066 commit d83f062
Show file tree
Hide file tree
Showing 2 changed files with 54 additions and 34 deletions.
43 changes: 36 additions & 7 deletions src/components/radio/radio.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,11 @@ describe('MdRadio', () => {

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();

expect(radioInstances.every(radio => !radio.checked)).toBe(true);
});
});
Expand Down Expand Up @@ -236,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);
Expand Down Expand Up @@ -333,7 +357,7 @@ describe('MdRadio', () => {
@Component({
directives: [MD_RADIO_DIRECTIVES],
template: `
<md-radio-group [disabled]="isGroupDisabled" [value]="groupValue">
<md-radio-group [disabled]="isGroupDisabled" [value]="groupValue" name="test-name">
<md-radio-button value="fire">Charmander</md-radio-button>
<md-radio-button value="water">Squirtle</md-radio-button>
<md-radio-button value="leaf">Bulbasaur</md-radio-button>
Expand Down Expand Up @@ -365,21 +389,26 @@ class StandaloneRadioButtons { }
directives: [MD_RADIO_DIRECTIVES, FORM_DIRECTIVES],
template: `
<md-radio-group [(ngModel)]="modelValue">
<md-radio-button value="vanilla">Vanilla</md-radio-button>
<md-radio-button value="chocolate">Chocolate</md-radio-button>
<md-radio-button value="strawberry">Strawberry</md-radio-button>
<md-radio-button *ngFor="let option of options" [value]="option.value">
{{option.label}}
</md-radio-button>
</md-radio-group>
`
})
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.
*/
Expand Down
45 changes: 18 additions & 27 deletions src/components/radio/radio.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -160,10 +153,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.
Expand All @@ -181,9 +170,15 @@ 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, 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) {
Expand All @@ -192,8 +187,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; });
}
}
}
Expand All @@ -206,7 +201,7 @@ export class MdRadioGroup implements AfterContentInit, ControlValueAccessor {
this.change.emit(event);
}

/**
/**
* Implemented as part of ControlValueAccessor.
* @internal
*/
Expand Down Expand Up @@ -258,7 +253,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()
Expand Down Expand Up @@ -345,15 +340,11 @@ 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) {
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;
}
}

Expand Down

0 comments on commit d83f062

Please sign in to comment.