-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Conversation
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4333 +/- ##
===================================================
+ Coverage 98.52% 98.53% +<.01%
===================================================
Files 93 93
Lines 5832 5851 +19
Branches 789 788 -1
===================================================
+ Hits 5746 5765 +19
Misses 85 85
Partials 1 1
Continue to review full report at Codecov.
|
const isClick = evt.type === 'click'; | ||
const isEnter = evt.key === 'Enter' || evt.keyCode === 13; | ||
const isEnter = (evt as KeyboardEvent).key === 'Enter' || (evt as KeyboardEvent).keyCode === 13; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract these checks to separate functions to make the code more readable. Ditto below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ken wasn't wild about extracting these out into util
functions, so I reverted that change.
import {MDCRipple} from '@material/ripple/index'; | ||
|
||
import * as createFocusTrap from 'focus-trap'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use two separate imports: one for the function, and one for the type definitions:
import createFocusTrap from 'focus-trap';
import * as FocusTrapLib from 'focus-trap';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you suggesting we shouldn't need to re-export FocusTrapLib from our own ./types
(which is where we're currently importing it from)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we need to do it this way:
import * as createFocusTrap from 'focus-trap';
// Function signature
createFocusTrap(...);
// Interface definitions
createFocusTrap.FocusTrap;
createFocusTrap.Options;
Although Focus Trap's .d.ts
file claims to have a default export, default imports don't work:
import createFocusTrap from 'focus-trap';
// ERROR: undefined is not a function
createFocusTrap(...);
All 621 screenshot tests passed for commit e339ccf vs. |
All 621 screenshot tests passed for commit 19b28b5 vs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
All 621 screenshot tests passed for commit a1cbcc4 vs. |
related #4225