-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(radio): support ngModel on md-radio-group #209
feat(radio): support ngModel on md-radio-group #209
Conversation
doctorrustynelson
commented
Mar 19, 2016
- Added support for ngModel on md-radio-group
- Updated Documentation
- Added Demo App Example
This should fix #208 that I submitted earlier. |
/** Event emitted when the group value changes. */ | ||
@Output() | ||
change: EventEmitter<MdRadioChange> = new EventEmitter(); | ||
change: EventEmitter<any> = new EventEmitter(); |
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.
Why are you removing the event object?
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.
I changed the type of the event from the MdRadioChange to just the value of the radio button because I found that if I didn't the ngModel I would bind to would get an MdRadioChange event as it's value instead of the intended value. The changes related to this line up with similar code that I saw in the MdCheckbox implementation which I used as a reference for most of my 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.
We should keep the event object (checkbox should also have one eventually), and make it so that you just call the fn
argument with the value extracted from the event object:
registerOnChange(fn: any) {
if (this._changeSubscription) {
this._changeSubscription.unsubscribe();
}
this._changeSubscription = <{unsubscribe: () => any}>this.change.subscribe(changeEvent => {
fn(changeEvent.value);
});
}
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.
I have re-included the MdRadioChange event as requested.
Changes seem good, just needs unit tests now. |
I have cleaned up the code as suggested and added a unit test. |
@@ -210,6 +252,10 @@ export class MdRadioButton implements OnInit { | |||
if (this.id == null) { | |||
this.id = `md-radio-${_uniqueIdCounter++}`; | |||
} | |||
|
|||
if (this.radioGroup && this._value == this.radioGroup.value) { |
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.
Looks like this added behavior also needs a test.
LGTM aside from one more unit test. cc @rfru |
@doctorrustynelson forgot one last thing: can you squash your commits into one? The commit message should be something like
Thanks! |
78ac0a2
to
4c22501
Compare
@jelbourn Thank you for all the suggestions; I have done as requested and squashed the commits. |
LGTM |
Thanks for your contribution! |
* Includes the `Roboto` font from Google Fonts. Previously only `Roboto Mono` was included. * Includes the Web animations polyfill in order for the component animations to work on all browsers.
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. |