-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(linear-progress): Convert JS to TypeScript #4272
feat(linear-progress): Convert JS to TypeScript #4272
Conversation
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.
I am also getting lint errors, and some tests errors. Looks like unit tests are not running on this branch since it branches from mdc-animation conversion branch
getPrimaryBar: () => null, | ||
hasClass: (_className: string) => false, | ||
removeClass: (_className: string) => {}, | ||
setStyle: (_el: HTMLElement, _styleProperty: string, _value: string) => {}, |
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.
is the _styleProperty
better suited as type [key: CSSStyleDeclaration]
?
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.
I don't think we can't use keyof CSSStyleDeclaration
here because it lacks vendor prefixes:
const transformStyleProperties = ['transform', 'WebkitTransform', 'MozTransform', 'OTransform', 'MSTransform']; |
material-components-web/packages/mdc-linear-progress/foundation.ts
Lines 106 to 108 in 3a874f5
transformStyleProperties.forEach((transformStyleProperty) => { | |
this.adapter_.setStyle(el, transformStyleProperty, value); | |
}); |
setStyle: (el: HTMLElement, styleProperty: string, value: string) => el.style.setProperty(styleProperty, value), |
I'm pretty sure we can get rid of transformStyleProperties
because all major browsers now implement transform
either unprefixed or -webkit-
prefixed, according to Can I Use.
Even so, getCorrectPropertyName()
returns StandardCssPropertyName | PrefixedCssPropertyName
, which is not directly type-compatible with
keyof CSSStyleDeclaration
.
WDYT?
return new MDCLinearProgress(root); | ||
} | ||
|
||
set determinate(value) { | ||
set determinate(value: boolean) { |
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.
ts compiler says it is returning a boolean. Is that correct?
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.
No; I tried adding a return value and got a TypeScript error:
TS2408: Setters cannot return a value
…tion--linear-progress
…nt` (to support wrapper frameworks)
Codecov Report
@@ Coverage Diff @@
## feat/typescript #4272 +/- ##
===================================================
+ Coverage 98.57% 98.57% +<.01%
===================================================
Files 92 92
Lines 5665 5674 +9
Branches 757 760 +3
===================================================
+ Hits 5584 5593 +9
Misses 81 81
Continue to review full report at Codecov.
|
All 758 screenshot tests passed for commit 55e3642 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.
One comment, but looks good to me otherwise.
All 758 screenshot tests passed for commit 2eb71c9 vs. |
Refs #4225