feat(drawer): Convert JS to TypeScript #4390

acdvorak · 2019-02-09T06:59:59Z

codecov-io · 2019-02-09T07:07:58Z

Codecov Report

Merging #4390 into feat/typescript will increase coverage by 0.22%.
The diff coverage is 100%.

@@                Coverage Diff                 @@
##           feat/typescript   #4390      +/-   ##
==================================================
+ Coverage            98.68%   98.9%   +0.22%     
==================================================
  Files                   94      95       +1     
  Lines                 5987    6109     +122     
  Branches               805     802       -3     
==================================================
+ Hits                  5908    6042     +134     
+ Misses                  78      66      -12     
  Partials                 1       1

Impacted Files	Coverage Δ
packages/mdc-drawer/dismissible/foundation.ts	`100% <100%> (ø)`
packages/mdc-dialog/util.ts	`100% <100%> (ø)`	⬆️
packages/mdc-drawer/index.ts	`100% <100%> (ø)`
packages/mdc-dialog/index.ts	`100% <100%> (ø)`	⬆️
packages/mdc-drawer/modal/foundation.ts	`100% <100%> (ø)`
packages/mdc-drawer/util.ts	`100% <100%> (ø)`
packages/mdc-base/component.ts	`96.42% <0%> (ø)`	⬆️
packages/mdc-textfield/character-counter/index.js
packages/mdc-select/icon/foundation.js
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2fa9a5...1cd4d00. Read the comment docs.

mdc-web-bot · 2019-02-09T07:15:45Z

All 621 screenshot tests passed for commit ed69731 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-09T07:50:25Z

All 621 screenshot tests passed for commit c8e9df4 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-09T08:22:11Z

All 621 screenshot tests passed for commit e7719ba vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-09T21:14:54Z

All 621 screenshot tests passed for commit cf36082 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-10T17:26:42Z

All 621 screenshot tests passed for commit d53d1bc vs. feat/typescript! 💯🎉

packages/mdc-dialog/util.ts

test/unit/mdc-dialog/util.test.js

test/unit/mdc-drawer/dismissible.foundation.test.js

test/unit/mdc-drawer/mdc-drawer.test.js

test/unit/mdc-drawer/modal.foundation.test.js

mdc-web-bot · 2019-02-11T21:26:32Z

All 621 screenshot tests passed for commit 01d0276 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-11T21:31:38Z

All 621 screenshot tests passed for commit d6d7132 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-11T21:36:09Z

All 621 screenshot tests passed for commit b6f32cd vs. feat/typescript! 💯🎉

…cript--drawer

mdc-web-bot · 2019-02-11T23:53:27Z

All 624 screenshot tests passed for commit b152591 vs. feat/typescript! 💯🎉

mdc-web-bot · 2019-02-12T04:33:56Z

All 624 screenshot tests passed for commit 24f442c vs. feat/typescript! 💯🎉

…cript--drawer

mdc-web-bot · 2019-02-12T05:34:36Z

All 624 screenshot tests passed for commit b34993e vs. feat/typescript! 💯🎉

packages/mdc-drawer/dismissible/foundation.ts

moog16 · 2019-02-12T20:23:47Z

packages/mdc-drawer/dismissible/foundation.ts

    const {OPENING, CLOSING, OPEN, ANIMATE, ROOT} = cssClasses;

    // In Edge, transitionend on ripple pseudo-elements yields a target without classList, so check for Element first.
    const isElement = evt.target instanceof Element;
-    if (!isElement || !this.adapter_.elementHasClass(/** @type {!Element} */ (evt.target), ROOT)) {
+    if (!isElement || !this.adapter_.elementHasClass(evt.target as Element, ROOT)) {


This would be a good place for a guard:

function isElement(element: any): element is Element { return element instanceof Element; } if (!this.adapter_.elementHasClass(isElement(evt.target), ROOT)

Done. instanceof checks make it harder to mock objects in unit tests, so I'm testing for the presence of classList instead.

class MDCDismissibleDrawerFoundation { handleTransitionEnd(evt: TransitionEvent) { // In Edge, transitionend on ripple pseudo-elements yields a target without classList, so check for Element first. const isRootElement = this.isElement_(evt.target) && this.adapter_.elementHasClass(evt.target, ROOT); if (!isRootElement) { return; } } private isElement_(element: unknown): element is Element { // In Edge, transitionend on ripple pseudo-elements yields a target without classList. return Boolean((element as Element).classList); } }

packages/mdc-drawer/dismissible/foundation.ts

packages/mdc-drawer/index.ts

moog16 · 2019-02-12T20:30:15Z

packages/mdc-drawer/index.ts

+  private scrim_!: Element | null; // assigned in initialSyncWithDOM()
+  private list_?: MDCList; // assigned in initialize()
+
+  private focusTrap_?: createFocusTrap.FocusTrap; // assigned in initialSyncWithDOM()


I think change this to focusTrap_!, then below we don't have to the do focusTrap_!.active...

focusTrap_ is undefined if the scrim is not present:

initialSyncWithDOM() { const {MODAL} = MDCDismissibleDrawerFoundation.cssClasses; const {SCRIM_SELECTOR} = MDCDismissibleDrawerFoundation.strings; this.scrim_ = (this.root_.parentNode as Element).querySelector<HTMLElement>(SCRIM_SELECTOR); if (this.scrim_ && this.root_.classList.contains(MODAL)) { this.handleScrimClick_ = () => (this.foundation_ as MDCModalDrawerFoundation).handleScrimClick(); this.scrim_.addEventListener('click', this.handleScrimClick_); this.focusTrap_ = util.createFocusTrapInstance(this.root_ as HTMLElement, this.focusTrapFactory_); } }

So we have to keep focusTrap_? and focusTrap_!.active

mdc-web-bot · 2019-02-13T00:08:35Z

All 624 screenshot tests passed for commit b5a06e5 vs. feat/typescript! 💯🎉

kfranqueiro · 2019-02-13T18:03:18Z

packages/mdc-dialog/index.ts

@@ -70,7 +70,7 @@ class MDCDialog extends MDCComponent<MDCDialogFoundation> {
  private container_!: HTMLElement; // assigned in initialize()
  private content_!: HTMLElement | null; // assigned in initialize()
  private defaultButton_!: HTMLElement | null; // assigned in initialize()
-  private initialFocusEl_!: HTMLElement | null; // assigned in initialize()
+  private initialFocusEl_!: HTMLElement | undefined; // assigned in initialize()


This seems like a contradiction in terms. Isn't this effectively private initialFocusEl_?: HTMLElement?

Confusingly, the ! here has a slightly different meaning than when it's used after an expression.

When declaring member properties, ! is a "definite assignment assertion", meaning "I pinky swear that some value is assigned to this property, but it happens somewhere other than the constructor.

In an expression (e.g., querySelector('.foo')!), the bang means "this value will never be null or undefined."

You make a good point though; this can be shortened to private initialFocusEl_?: HTMLElement.

kfranqueiro · 2019-02-13T18:05:23Z

packages/mdc-dialog/index.ts

@@ -93,7 +93,7 @@ class MDCDialog extends MDCComponent<MDCDialogFoundation> {
    this.buttons_ = [].slice.call(this.root_.querySelectorAll<HTMLElement>(strings.BUTTON_SELECTOR));
    this.defaultButton_ = this.root_.querySelector<HTMLElement>(strings.DEFAULT_BUTTON_SELECTOR);
    this.focusTrapFactory_ = focusTrapFactory;
-    this.initialFocusEl_ = initialFocusEl as HTMLElement;
+    this.initialFocusEl_ = initialFocusEl as HTMLElement | undefined;


Why do we cast to HTMLElement here when we're accepting Element above? Is there something we need from HTMLElement in particular?

I was trying to make things simpler for Closure users (who typically pass an Element).

However, this method is called indirectly via the MDCComponent constructor (which takes an Element), so it actually shouldn't be an issue for them.

I'll change the type of the initialFocusEl arg to HTMLElement. The TypeScript compiler seems fine with it.

kfranqueiro · 2019-02-13T18:09:20Z

packages/mdc-drawer/types.ts

+import {MDCList} from '@material/list/index';
+import * as FocusTrapLib from 'focus-trap';
+
+// TODO(acdvorak): Centralize this in mdc-base or mdc-dom?


RE this TODO: I wouldn't want to centralize a third-party dependency in a package that is depended upon by a large majority of our packages. Right now it's only a dependency of 2 packages.

Fair point. I'll remove it.

mdc-web-bot · 2019-02-14T01:07:05Z

All 624 screenshot tests passed for commit 1cd4d00 vs. feat/typescript! 💯🎉

Addressed Ken's comments; Matt approved

feat(drawer): Convert JS to TypeScript

ed69731

Refs #4225

acdvorak assigned kfranqueiro and moog16 Feb 9, 2019

acdvorak requested review from kfranqueiro and moog16 February 9, 2019 07:00

googlebot added the cla: yes label Feb 9, 2019

WIP: Increase unit test coverage to 100%

c8e9df4

kfranqueiro mentioned this pull request Feb 9, 2019

Convert packages to TypeScript #4225

Closed

45 tasks

WIP: Fix linter error

e7719ba

WIP: Make Istanbul happy

cf36082

WIP: Make Istanbul happy

d53d1bc

kfranqueiro suggested changes Feb 11, 2019

View reviewed changes

acdvorak added 5 commits February 11, 2019 13:06

WIP: Add explicit nullable to type

01d0276

WIP: Make initialFocusEl optional instead of nullable

53a46ee

WIP: Remove inaccurate # from util.test.js

d6d7132

WIP: Update activeElement equality check in unit test

b6f32cd

WIP: Revert unnecessary change to unit test

d3240f8

Merge remote-tracking branch 'origin/feat/typescript' into feat/types…

b152591

…cript--drawer

WIP: Fix linter error

24f442c

Merge remote-tracking branch 'origin/feat/typescript' into feat/types…

b34993e

…cript--drawer

moog16 suggested changes Feb 12, 2019

View reviewed changes

WIP: Address review comments

b5a06e5

moog16 approved these changes Feb 13, 2019

View reviewed changes

kfranqueiro previously requested changes Feb 13, 2019

View reviewed changes

WIP: Address review comments

1cd4d00

acdvorak merged commit f8ba48f into feat/typescript Feb 14, 2019

acdvorak deleted the feat/typescript--drawer branch February 14, 2019 03:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(drawer): Convert JS to TypeScript #4390

feat(drawer): Convert JS to TypeScript #4390

acdvorak commented Feb 9, 2019

codecov-io commented Feb 9, 2019 •

edited

Loading

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 10, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 12, 2019

mdc-web-bot commented Feb 12, 2019

moog16 Feb 12, 2019

acdvorak Feb 12, 2019

moog16 Feb 12, 2019

acdvorak Feb 12, 2019

mdc-web-bot commented Feb 13, 2019

kfranqueiro Feb 13, 2019

acdvorak Feb 14, 2019

acdvorak Feb 14, 2019

kfranqueiro Feb 13, 2019

acdvorak Feb 14, 2019

acdvorak Feb 14, 2019

kfranqueiro Feb 13, 2019

acdvorak Feb 14, 2019

acdvorak Feb 14, 2019

mdc-web-bot commented Feb 14, 2019

feat(drawer): Convert JS to TypeScript #4390

feat(drawer): Convert JS to TypeScript #4390

Conversation

acdvorak commented Feb 9, 2019

codecov-io commented Feb 9, 2019 • edited Loading

Codecov Report

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 9, 2019

mdc-web-bot commented Feb 10, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 11, 2019

mdc-web-bot commented Feb 12, 2019

mdc-web-bot commented Feb 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Feb 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdc-web-bot commented Feb 14, 2019

codecov-io commented Feb 9, 2019 •

edited

Loading