feat(menu-surface): Convert JS to TypeScript #4273
Conversation
acdvorak
force-pushed
the
feat/typescript--menu-surface
branch
from
7b1d930
to
6324518
Compare
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4273 +/- ##
===================================================
+ Coverage 98.36% 98.45% +0.09%
===================================================
Files 93 93
Lines 5736 5747 +11
Branches 778 775 -3
===================================================
+ Hits 5642 5658 +16
+ Misses 94 89 -5
Continue to review full report at Codecov.
|
|
All 758 screenshot tests passed for commit 7b1d930 vs. |
|
All 758 screenshot tests passed for commit 6324518 vs. |
acdvorak
changed the title
packages/mdc-menu-surface/index.ts
Outdated
| this.foundation_.setQuickOpen(quickOpen); | ||
| } | ||
| getDefaultFoundation(): MDCMenuSurfaceFoundation { | ||
| // TODO(acdvorak): Should we change the type of root_ to HTMLElement? What other kind of element could it be? |
There was a problem hiding this comment.
Ya I was reading the differences between element and htmlelement. HTMLElement is a subset of Element. So element Element could be XML/SVG/HTML. I think in our case it will never be an XML element. I think its most accurate to be HTMLElement.
There was a problem hiding this comment.
The only situation I can think of where we might have a non-HTML element is ripples on SVG icons. However, SVGElement appears to have most of the same properties and methods as HTMLElement (style, querySelector(), etc.), so it's probably safe to update MDCComponent's root_ property type to HTMLElement.
WDYT?
There was a problem hiding this comment.
I remember when we were annotating for Closure compiler, HTMLElement was more problematic, possibly due to many DOM APIs applying more generically to (and returning) Elements. I'd be curious if we end up running into similar problems here or not.
There was a problem hiding this comment.
For now, I'm going to stick with Element and do type assertions where necessary (e.g., this.root_.style). Seems like the least risky option.
…` from properties
|
All 758 screenshot tests passed for commit 913f607 vs. |
|
All 758 screenshot tests passed for commit f55703f vs. |
…to `constants.ts`
There was a problem hiding this comment.
I think it looks good as is. But after chatting with @mmalerba we need to add some new properties to the tsconfig. Could you add strict: true to the tsconfig.json file and see what new errors pop up? I'm working on finding out what else we need to add.
|
All 621 screenshot tests passed for commit 63845ba vs. |
This reverts commit e264819.
|
All 621 screenshot tests passed for commit f00a4aa vs. |
| BOTTOM_END = CornerBit.BOTTOM | CornerBit.RIGHT | CornerBit.FLIP_RTL, // tslint:disable-line:no-bitwise | ||
| } | ||
|
|
||
| interface MenuDimensions { |
There was a problem hiding this comment.
Is constants.ts where we've been putting this sort of thing? It seems like an odd idea for interfaces to be here. Would putting them in adapter.ts make more sense?
There was a problem hiding this comment.
I went back and forth on that myself. @moog16 and I decided to move them from adapter.ts to constants.ts because the constants file already has enums, which feel "type-y".
We were thinking that any type whose name beings with MDC (i.e., index, foundation, and adapter) should have its own file; everything else would go in constants.js. It feels simple and straightforward to me.
We could conceivably create an entirely separate file just for interfaces and type definitions, but that might be overkill.
WDYT?
There was a problem hiding this comment.
IMO interfaces feel more type-y than enums (which are much more value-y than type-y at least the way we're using them), which is why I thought of adapter.ts (also since most of the interfaces currently defined in constants.ts are required by the adapter anyway).
How many components have we done this with so far?
There was a problem hiding this comment.
As discussed offline, we're going to create a new types.ts file for interfaces and other type definitions that don't belong in constants.ts or adapter.ts.
|
All 621 screenshot tests passed for commit 56b75ac vs. |
|
All 621 screenshot tests passed for commit d45afea vs. |
|
All 621 screenshot tests passed for commit 9c76326 vs. |
…ADME and component JS
|
All 621 screenshot tests passed for commit b964b75 vs. |
Accepting `string` values implies that we support all valid CSS units (`em`, `rem`, `%`, etc.), but in truth we only support `px`. Changing the type of the properties from `string` to `number` makes this clearer, and also simplifies converting the package to TypeScript in PR #4273. BREAKING CHANGE: `MDCMenuSurfaceAdapter.setPosition()` now expects an object with properties of type `number` rather than `string`. E.g., `setPosition({top: '5px', left: '10px'})` is now `setPosition({top: 5, left: 10})`.
Accepting `string` values implies that we support all valid CSS units (`em`, `rem`, `%`, etc.), but in truth we only support `px`. Changing the type of the properties from `string` to `number` makes this clearer, and also simplifies converting the package to TypeScript in PR #4273. BREAKING CHANGE: `MDCMenuSurfaceAdapter.setPosition()` now expects an object with properties of type `number` rather than `string`. E.g., `setPosition({top: '5px', left: '10px'})` is now `setPosition({top: 5, left: 10})`.
|
All 621 screenshot tests passed for commit 86cfdad vs. |
…ues (#4351) Accepting `string` values implies that we support all valid CSS units (`em`, `rem`, `%`, etc.), but in truth we only support `px`. Changing the type of the properties from `string` to `number` makes this clearer, and also simplifies converting the package to TypeScript in PR #4273. BREAKING CHANGE: `MDCMenuSurfaceAdapter.setPosition()` now expects an object with properties of type `number` rather than `string`. E.g., `setPosition({top: '5px', left: '10px'})` is now `setPosition({top: 5, left: 10})`.
kfranqueiro
dismissed
moog16’s stale review
Review comments have been resolved and Matt's not back until tomorrow
Refs #4225