feat(top-app-bar): Convert JS to TypeScript #4397
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4397 +/- ##
===================================================
+ Coverage 98.86% 98.93% +0.06%
===================================================
Files 95 95
Lines 6096 6117 +21
Branches 802 806 +4
===================================================
+ Hits 6027 6052 +25
+ Misses 68 64 -4
Partials 1 1
Continue to review full report at Codecov.
|
|
All 621 screenshot tests passed for commit e196d6b vs. |
|
All 621 screenshot tests passed for commit df96ff8 vs. |
|
All 621 screenshot tests passed for commit f85caa9 vs. |
…cript--top-app-bar
|
All 624 screenshot tests passed for commit ebaa080 vs. |
…cript--top-app-bar
|
All 624 screenshot tests passed for commit d90cf29 vs. |
|
|
||
| this.scrollHandler_ = () => this.fixedScrollHandler_(); | ||
| } | ||
|
|
||
| init() { | ||
| super.init(); | ||
| this.adapter_.registerScrollHandler(this.scrollHandler_); |
There was a problem hiding this comment.
why did you decide to remove this here?
There was a problem hiding this comment.
It's redundant; it already happens in MDCTopAppBarBaseFoundation.
The duplication was actually causing unit tests to fail until I removed this.
|
All 624 screenshot tests passed for commit c8fc735 vs. |
|
All 624 screenshot tests passed for commit 2a49df9 vs. |
| getViewportScrollY: () => { | ||
| const win = this.scrollTarget_ as Window; | ||
| const el = this.scrollTarget_ as Element; | ||
| return win.pageYOffset !== undefined ? win.pageYOffset : el.scrollTop; |
There was a problem hiding this comment.
Is there a reason this switched from a direct comparison to window to a duck-type?
There was a problem hiding this comment.
To make it more unit-testing friendly. Direct === comparisons and instanceof checks make it harder to mock things, especially if/when we convert the tests themselves to TypeScript.
| registerScrollHandler: (handler) => this.scrollTarget_.addEventListener('scroll', handler), | ||
| deregisterScrollHandler: (handler) => this.scrollTarget_.removeEventListener('scroll', handler), | ||
| notifyNavigationIconClicked: () => this.emit(strings.NAVIGATION_EVENT, {}), | ||
| registerScrollHandler: (handler) => this.scrollTarget_.addEventListener('scroll', handler as EventListener), |
There was a problem hiding this comment.
I don't think you need to cast them to EventListeners here.
There was a problem hiding this comment.
Removing them throws an error:
[tsl] ERROR in packages/mdc-top-app-bar/index.ts(92,89)
TS2345: Argument of type 'SpecificEventListener<"scroll">' is not assignable to parameter of type 'EventListener | EventListenerObject | null'.
Type 'SpecificEventListener<"scroll">' is not assignable to type 'EventListener'.
Types of parameters 'evt' and 'evt' are incompatible.
Type 'Event' is missing the following properties from type 'UIEvent': detail, view, initUIEvent
That's because scrollTarget_ is an EventTarget rather than an Element, and EventTarget doesn't have generic signatures for addEventListener() or removeEventListener() in lib.dom.d.ts (whereas Element does).
There was a problem hiding this comment.
I see - its because we need to set scrollTarget to EventTarget - mainly because it could be the Window.
|
All 624 screenshot tests passed for commit e3e9023 vs. |
| this.iconRipples_; | ||
| /** @type {Object} */ | ||
| this.scrollTarget_; | ||
| export type MDCRippleFactory = (el: Element) => MDCRipple; |
There was a problem hiding this comment.
I guess this will end up removed in #4407 after this is merged?
There was a problem hiding this comment.
Correct 😀 Thanks for the reminder!
Refs #4225