-
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): 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!