feat(checkbox): Annotate mdc-checkbox for closure #867
Conversation
Codecov Report
@@ Coverage Diff @@
## master #867 +/- ##
==========================================
- Coverage 98.58% 98.31% -0.27%
==========================================
Files 70 71 +1
Lines 3251 3266 +15
Branches 393 393
==========================================
+ Hits 3205 3211 +6
- Misses 46 55 +9
Continue to review full report at Codecov.
|
| @@ -14,8 +14,10 @@ | |||
| * limitations under the License. | |||
| */ | |||
|
|
|||
| /** @const {string} */ | |||
| const ROOT = 'mdc-checkbox'; | |||
There was a problem hiding this comment.
is ROOT still referenced somewhere?
There was a problem hiding this comment.
Indeed it's not. Removed!
packages/mdc-checkbox/foundation.js
Outdated
| @@ -220,6 +257,10 @@ export default class MDCCheckboxFoundation extends MDCFoundation { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * @return {!HTMLInputElement|InputPropsType} | |||
There was a problem hiding this comment.
This should just return InputPropsType. HTMLInputElement has getters for checked, indeterminate, disabled, and value.
Why does getNativeControl_ return a fake HTMLInputElement anyway? I know that is outside the scope of this PR...but it seems to be affecting how we annotate the method.
There was a problem hiding this comment.
Removed HTMLInputElement and moved InputPropsType def to the adapter so that the result of querySelector() can be casted to this type in the component class.
Returning that fake element seems to be required by some unit tests verifying the getters still work when no native control is returned.
packages/mdc-checkbox/foundation.js
Outdated
| * value: ?string | ||
| * }} | ||
| */ | ||
| let InputPropsType; |
There was a problem hiding this comment.
I prefer InputElementState...but I'd like to first know why we return a fake HTMLInputElement in getNativeControl_
There was a problem hiding this comment.
Thanks for the naming suggestion :)
packages/mdc-ripple/index.js
Outdated
| * disabled: (boolean|undefined) | ||
| * }} | ||
| */ | ||
| let RippleAbleType; |
There was a problem hiding this comment.
I talked to our MD team a bit, and I think "RippleCapableSurface" is more clear. Can you add a link to our guidelines on when to use ripples:
https://material.io/guidelines/motion/choreography.html#choreography-radial-reaction
Can you also add a note about what "unbounded" and "disabled" means from a ripple's point of view.
There was a problem hiding this comment.
Done. I changed from @typedef to @record so that those comments can be inlined.
|
Thanks for the review Lynn. I've also used the |
|
Sorry for the long delay, I was on vacation. This generally looks good, so I'm going to update the branch and make sure all tests still pass. If they do, I will merge! |
Fixes #334