-
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
feat(textfield): Annotate textfield for Closure Compiler. #1386
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1386 +/- ##
==========================================
+ Coverage 99.91% 99.91% +<.01%
==========================================
Files 69 69
Lines 3381 3390 +9
Branches 421 422 +1
==========================================
+ Hits 3378 3387 +9
Misses 3 3
Continue to review full report at Codecov.
|
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.
Overall LGTM, just a few minor comments and a question
packages/mdc-textfield/adapter.js
Outdated
* property, so if you choose to duck-type the return value for this method | ||
* in your implementation it's important to keep this in mind. Also note that | ||
* this method can return null, which the foundation will handle gracefully. | ||
* @return {HTMLInputElement|NativeInputType} |
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.
Add explicit nullable operator(s):
@return {?HTMLInputElement|?NativeInputType}
or
@return {!HTMLInputElement|!NativeInputType|null}
or
@return {HTMLInputElement|NativeInputType|null}
(I'm not sure which variation is more readable, correct, and Closure-friendly...)
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. I think null
is generally discouraged when ?
would suffice, so I went with {?A|?B}
.
packages/mdc-textfield/foundation.js
Outdated
getNativeInput_() { | ||
return this.adapter_.getNativeInput() || { | ||
return this.adapter_.getNativeInput() || | ||
/** @type {NativeInputType} */ ({ |
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.
Add explicit nullability operator:
@type {!NativeInputType}
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, found some more and fixed throughout.
@@ -103,6 +156,15 @@ export class MDCTextfield extends MDCComponent { | |||
}; | |||
} | |||
|
|||
/** | |||
* @return {!{ |
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 we create @record
definitions for these collections of methods?
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.
These subsets of the adapter API are kind of arbitrary (I assume we only split it like this as an attempt at readability); the only thing that really matters is the collective adapter API. Kind of unfortunate that we have to annotate these at all...
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.
Yes, because they are arbitrary subsets of the adapter API, it didn't make sense to make individual @record
or @typedef
s for these. Also since these methods are private, the type wouldn't be shared across components like for NativeInputType
.
Annotates MDC Textfield for the Closure Compiler.
Resolves #345