feat(text-field): Convert JS to TypeScript #4377
Conversation
acdvorak
force-pushed
the
feat/typescript--textfield
branch
from
cea3fad to
3e01b8c
Compare
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4377 +/- ##
===================================================
+ Coverage 98.68% 98.85% +0.17%
===================================================
Files 94 94
Lines 5987 6051 +64
Branches 805 810 +5
===================================================
+ Hits 5908 5982 +74
+ Misses 78 68 -10
Partials 1 1
Continue to review full report at Codecov.
|
|
All 624 screenshot tests passed for commit 8e34f91 vs. |
|
All 624 screenshot tests passed for commit 44f2987 vs. |
|
All 624 screenshot tests passed for commit 1c78cf6 vs. |
|
All 624 screenshot tests passed for commit 730164d vs. |
|
All 624 screenshot tests passed for commit 1d88400 vs. |
| */ | ||
| isNativeInputValid_() { | ||
| private isBadInput_(): boolean { | ||
| // The badInput property is not supported in IE 11 💩. |
There was a problem hiding this comment.
I dunno how we feel about your autocompleted emoji ending up in source :P
There was a problem hiding this comment.
Hah I guess I've gotten used to it and didn't even notice... Personally I think it's hilarious and fitting, but I can remove it if you really want 😛
| private input_!: HTMLInputElement; // assigned in initialize() | ||
|
|
||
| // Optional sub-elements. | ||
| private characterCounter_!: MDCTextFieldCharacterCounter | null; // assigned in initialize() |
There was a problem hiding this comment.
I notice all of these are ! and | null, but you also replaced the ifs below in initialize with ternary expressions. Could these be ? (and not require | null) if we left initialize more intact with ifs? Did doing it this way make anything easier?
There was a problem hiding this comment.
Using null just made the code a little cleaner by condensing the if statements down to ternaries. That's the only reason I did it 😄
There was a problem hiding this comment.
I guess since they're all private it doesn't really matter either way. I'm not sure if there's any perception that one is preferable over the other, typing-wise, other than ? existing as a more concise representation of | undefined.
test/unit/mdc-textfield/mdc-text-field-character-counter-foundation.test.js
Outdated
Show resolved
Hide resolved
|
All 624 screenshot tests passed for commit 530c4c6 vs. |
|
All 624 screenshot tests passed for commit be64af2 vs. |
| import {MDCTextFieldIconFoundation} from './icon/foundation'; | ||
| import {MDCTextFieldIcon} from './icon/index'; | ||
|
|
||
| export type NativeInputElement = Pick<HTMLInputElement, 'disabled' | 'maxLength' | 'type' | 'value'> & { |
There was a problem hiding this comment.
Thanks for the suggestion! 🎉 🌮
|
All 624 screenshot tests passed for commit c405d56 vs. |
|
All 624 screenshot tests passed for commit 03f0022 vs. |
acdvorak
dismissed
kfranqueiro’s stale review
Addressed Ken's concerns about unit tests; approved by Matt
Refs #4225