feat(top-app-bar): typescript #548
Conversation
This reverts commit d486cef.
Codecov Report
@@ Coverage Diff @@
## feat/two-typescript #548 +/- ##
=======================================================
+ Coverage 96.6% 96.64% +0.03%
=======================================================
Files 60 60
Lines 2208 2233 +25
Branches 145 162 +17
=======================================================
+ Hits 2133 2158 +25
Misses 44 44
Partials 31 31
Continue to review full report at Codecov.
|
packages/top-app-bar/FixedAdjust.tsx
Outdated
| className?: string; | ||
| dense?: boolean; | ||
| prominent?: boolean; | ||
| short?: boolean |
| export default class TopAppBar extends React.Component { | ||
| foundation_ = null; | ||
| export interface TopAppBarProps { | ||
| actionItems?: React.ReactElement<any>[]; |
There was a problem hiding this comment.
Usually any is not allowed when using --noImplicitAny, which google3 uses. Is there any reason the generic type cannot be specified ahead of time?
There was a problem hiding this comment.
I think --noImplicitAny will allow you to imply any types, but defining it as any is ok. However it is the lazy way out. I think in this case, it'll be an or or
| > { | ||
| topAppBarElement: React.RefObject<HTMLElement> = React.createRef(); | ||
| foundation_?: MDCFixedTopAppBarFoundation | MDCTopAppBarFoundation | MDCShortTopAppBarFoundation; | ||
|
|
There was a problem hiding this comment.
Is this union useful for a 3p developer? If so, consider making a type for MDCTopAppBarFoundation which is the union of these 3
| @@ -12,7 +21,7 @@ class TopAppBarProminentToShortCollapsedScreenshotTest extends React.Component { | |||
| window.addEventListener('resize', this.updateTopAppBarVariant); | |||
| } | |||
|
|
|||
| shouldComponentUpdate(_, nextState) { | |||
| shouldComponentUpdate(_: {}, nextState: TopAppBarProminentToShortCollapsedScreenshotTestState) { | |||
There was a problem hiding this comment.
Consider @OverRide annotations (for readability)
| } from '../../../packages/top-app-bar'; | ||
|
|
||
| const MainTopAppBarContent: React.FunctionComponent<TopAppbarFixedAdjustProps> = (props) => { | ||
| return ( |
moog16
changed the base branch from
fix/coverage-typescript
to
feat/two-typescript
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
related #48