-
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
fix(checkbox, slide-toggle): only emit change event if native input emitted one. #820
Conversation
b5c5424
to
c79221c
Compare
@devversion could you rebase? |
c79221c
to
a2141c4
Compare
@jelbourn Done! |
@@ -214,13 +213,47 @@ describe('MdCheckbox', () => { | |||
expect(testComponent.onCheckboxClick).toHaveBeenCalledTimes(1); | |||
}); | |||
|
|||
it('should emit a change event when the `checked` value changes', async(() => { | |||
it('should trigger the change event properly', 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.
"Property" isn't very specific. How about "should trigger a change event when the native input does" ?
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.
Sounds good
LGTM besides the test name. @robertmesserle I think this fix covers what you were doing in #739, any thoughts? |
@jelbourn @devversion I think so. Does this fix the nested checkbox demo? I was using that as a test-case that I wanted to support. #575 has a good demo for what I was trying to fix. |
Yeah, one minor thing needs to be fixed, because the
I tested that out, and it seems to work as well, when doing the adjustments for the |
d847cf9
to
ed51258
Compare
LGTM, just needs a rebase. |
ed51258
to
482aff1
Compare
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
1 similar comment
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
482aff1
to
d9f0025
Compare
CLAs look good, thanks! |
1 similar comment
CLAs look good, thanks! |
d9f0025
to
e5b461b
Compare
@jelbourn Uff, I accidentally had the Slider PR from testing in there. but I was able to fix the branch, so it's now rebased again 😄 |
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. |
This makes sure that the given component matches the native browser behavior and doesn't emit a change event for example, when changing the native
checked
property.FYI: This change is also required for the
radio
component, but this should be addressed in another PR, since this will require a bit more work, through theradio
group.References #791