-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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): remove getNativeControl from adapter #3785
fix(radio): remove getNativeControl from adapter #3785
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3785 +/- ##
==========================================
- Coverage 98.52% 98.52% -0.01%
==========================================
Files 120 120
Lines 5236 5224 -12
Branches 658 657 -1
==========================================
- Hits 5159 5147 -12
Misses 77 77
Continue to review full report at Codecov.
|
All 557 screenshot tests passed for commit 1b0feca vs. |
All 560 screenshot tests passed for commit 92cb7c7 vs. |
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.
Just a few nits, otherwise LGTM!
packages/mdc-radio/README.md
Outdated
| `removeClass(className: string) => void` | Removes a class from the root element | | ||
Method Signature | Description | ||
--- | --- | ||
`setNativeControlDisabled() => void` | Sets the input to disabled |
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.
Missing disabled: boolean
argument.
Also update description to something like:
Sets the input's `disabled` property to the given value
assert.isTrue(radio.disabled); | ||
}); | ||
|
||
test('#adapter.setNativeControlDisabled returns false when checkbox is not disabled', () => { |
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 found the wording of this description a little confusing.
Maybe rephrase it to:
"#adapter.setNativeControlDisabled sets the native control element's disabled property to false"
assert.isFalse(root.classList.contains('foo')); | ||
}); | ||
|
||
test('#adapter.setNativeControlDisabled sets the native control element disabled', () => { |
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 found the wording of this description a little confusing.
Maybe rephrase it to:
"#adapter.setNativeControlDisabled sets the native control element's disabled property to true"
All 560 screenshot tests passed for commit bd18aef vs. |
BREAKING CHANGE: Removed getNativeControl from adapter, and subsequent foundation methods that called getNativeControl. Foundation methods removed: isChecked, setChecked, isDisabled, getValue, setValue.
fixes #3763
related #2684
BREAKING CHANGE: Removed
getNativeControl
from adapter, and subsequent foundation methods that calledgetNativeControl
. Foundation methods removed:isChecked
,setChecked
,isDisabled
,getValue
,setValue
.