feat(snackbar): Convert JS to TypeScript #4363
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4363 +/- ##
===================================================
+ Coverage 98.65% 98.65% +<.01%
===================================================
Files 93 93
Lines 5930 5947 +17
Branches 797 795 -2
===================================================
+ Hits 5850 5867 +17
Misses 79 79
Partials 1 1
Continue to review full report at Codecov.
|
|
All 621 screenshot tests passed for commit 5bfb6be vs. |
packages/mdc-snackbar/index.ts
Outdated
| this.surfaceEl_ = /** @type {!HTMLElement} */ (this.root_.querySelector(SURFACE_SELECTOR)); | ||
| this.labelEl_ = /** @type {!HTMLElement} */ (this.root_.querySelector(LABEL_SELECTOR)); | ||
| this.actionEl_ = /** @type {!HTMLElement} */ (this.root_.querySelector(ACTION_SELECTOR)); | ||
| this.surfaceEl_ = this.root_.querySelector<HTMLElement>(SURFACE_SELECTOR)!; |
There was a problem hiding this comment.
We're casting to HTMLElement here but defining these as Element above? If they were already safely HTMLElement in the original implementation, then we can probably preserve that... either way, should we be consistent?
There was a problem hiding this comment.
Removed HTMLElement generic type param
| * @param {!MouseEvent} evt | ||
| */ | ||
| handleActionButtonClick(evt) { | ||
| handleActionButtonClick(_evt: MouseEvent) { |
There was a problem hiding this comment.
can you remove this argument since you're not using it?
There was a problem hiding this comment.
I considered doing removing it, but I thought it would be more future-proof to keep it.
If we end up needing to inspect the event object in the future—e.g., to avoid taking an action if the shift/meta keys are pressed—we won't need to introduce a breaking change, because we'll already have access to the event.
Removing the param would technically be a breaking change too, so we'd have to create a separate PR to make the change in master as well.
There was a problem hiding this comment.
Yeah... from the perspective of this being a straight conversion, might as well leave it, since I guess it was unused even on master then, right?
packages/mdc-snackbar/index.ts
Outdated
| this.surfaceEl_ = /** @type {!HTMLElement} */ (this.root_.querySelector(SURFACE_SELECTOR)); | ||
| this.labelEl_ = /** @type {!HTMLElement} */ (this.root_.querySelector(LABEL_SELECTOR)); | ||
| this.actionEl_ = /** @type {!HTMLElement} */ (this.root_.querySelector(ACTION_SELECTOR)); | ||
| this.surfaceEl_ = this.root_.querySelector<HTMLElement>(SURFACE_SELECTOR)!; |
There was a problem hiding this comment.
I also notice you're ! to each querySelector call. Should we throw an error if they don't exist?
There was a problem hiding this comment.
I think we normally avoid throwing errors for required elements, except in very specific circumstances where it's easy for clients to make a mistake and hard for them to figure out what caused it.
E.g., modal/dismissible drawers need a scrim element, whereas permanent drawers lack one. If a client decides to switch from a permanent drawer to a dismissible drawer, they could very easily forget to add the scrim element, so a descriptive error message is really helpful.
This case is a little more straightforward because all the elements are always required.
Personally, I wouldn't be opposed to throwing errors for all missing elements, but we probably shouldn't make that change in this PR; we'd want to do that across all packages for consistency.
|
All 621 screenshot tests passed for commit 541b4e4 vs. |
|
All 621 screenshot tests passed for commit 4d06fd5 vs. |
|
All 621 screenshot tests passed for commit 9fd3bce vs. |
|
All 621 screenshot tests passed for commit ad978e3 vs. |
Refs #4225