-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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): update radio group when programmatically uncheck a radio button #1561
Conversation
@@ -216,6 +216,42 @@ describe('MdRadio', () => { | |||
|
|||
expect(radioInstances.every(radio => !radio.checked)).toBe(true); | |||
}); | |||
|
|||
it('should set radio group to null if uncheck a checked radio button programmatically', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should update the group's selected radio to null when unchecking that radio programmatically
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
expect(changeSpy).toHaveBeenCalledTimes(2); | ||
expect(groupInstance.value).toBeFalsy(); | ||
expect(radioInstances.every(radio => !radio.checked)).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you also expect(groupInstance.selected).toBeNull()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
expect(radioInstances.every(radio => !radio.checked)).toBe(true); | ||
}); | ||
|
||
it('should notify the group if check another radio button', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should fire a change event from the group whenever a radio checked state changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
this._checked = newCheckedState; | ||
|
||
if (newCheckedState && this.radioGroup && this.radioGroup.value != this.value) { | ||
this.radioGroup.selected = this; | ||
} else if (!newCheckedState && this.radioGroup && this.radioGroup.value == this.value) { | ||
// For programmatically uncheck radio button, notify the group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// When unchecking the selected radio button, update the selected radio
// property on the group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments addressed, PTAL. Thanks!
expect(radioInstances.every(radio => !radio.checked)).toBe(true); | ||
}); | ||
|
||
it('should notify the group if check another radio button', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
expect(changeSpy).toHaveBeenCalledTimes(2); | ||
expect(groupInstance.value).toBeFalsy(); | ||
expect(radioInstances.every(radio => !radio.checked)).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -216,6 +216,42 @@ describe('MdRadio', () => { | |||
|
|||
expect(radioInstances.every(radio => !radio.checked)).toBe(true); | |||
}); | |||
|
|||
it('should set radio group to null if uncheck a checked radio button programmatically', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
this._checked = newCheckedState; | ||
|
||
if (newCheckedState && this.radioGroup && this.radioGroup.value != this.value) { | ||
this.radioGroup.selected = this; | ||
} else if (!newCheckedState && this.radioGroup && this.radioGroup.value == this.value) { | ||
// For programmatically uncheck radio button, notify the group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
14a235b
to
cd66748
Compare
cd66748
to
6cc1981
Compare
LGTM |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
For issue #609