-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(radio): Radio component. #125
Conversation
} | ||
|
||
md-radio-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.
Empty definition?
Could you rebase? I have a feeling it will be green(er). |
Ready for another round. Will get started with tests soon. |
// TODO(mtlin): Replace when ink ripple component is implemented. | ||
// A placeholder ink ripple, shown when keyboard focused. | ||
.md-ink-ripple { | ||
background: #000; |
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 this be one of the theme background colors?
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.
I don't see any change here.
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.
This one's below, in the theme section.
fixture.detectChanges(); | ||
expect(button.componentInstance.checked).toBe(false); | ||
|
||
let event = new Event('change'); |
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.
Doesn't new Event
not work in IE11?
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.
Added a helper function for this.
Ready for another look. |
LGTM |
forwardRef | ||
} from 'angular2/core'; | ||
|
||
import {EventEmitter} from 'angular2/src/facade/async'; |
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 think you should be able to get this from 'angular2/core' as well, if you want to simplify imports
Updated with changes from @kara 's comments. |
Those are just flakes on Edge |
… new version of cli (angular#125)
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. |
@jelbourn @hansl @kara
Looking to get an initial pass first and then if it's looking close I'll update with tests.
Thanks!