-
Notifications
You must be signed in to change notification settings - Fork 32
Convert shared P5 to TypeScript #3563
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3563 +/- ##
=======================================
Coverage 98.91% 98.91%
=======================================
Files 806 806
Lines 14498 14499 +1
Branches 4119 4121 +2
=======================================
+ Hits 14341 14342 +1
Misses 150 150
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #3563 +/- ##
=======================================
Coverage 98.91% 98.91%
=======================================
Files 806 806
Lines 14498 14499 +1
Branches 4119 4121 +2
=======================================
+ Hits 14341 14342 +1
Misses 150 150
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3563 +/- ##
=======================================
Coverage 98.91% 98.91%
=======================================
Files 806 806
Lines 14498 14499 +1
Branches 4112 4114 +2
=======================================
+ Hits 14341 14342 +1
Misses 150 150
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will decrease total bundle size by 595 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Bundle ReportChanges will decrease total bundle size by 595 bytes (-0.0%) ⬇️. This is within the configured threshold ✅ Detailed changes
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. @@ Coverage Diff @@
## main #3563 +/- ##
=======================================
Coverage 98.90% 98.90%
=======================================
Files 806 806
Lines 14484 14485 +1
Branches 4115 4117 +2
=======================================
+ Hits 14326 14327 +1
Misses 151 151
Partials 7 7
Continue to review full report in Codecov by Sentry.
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
44690be
to
6c6b0d8
Compare
import A from 'ui/A' | ||
import Button from 'ui/Button' | ||
import { Card } from 'ui/Card' | ||
|
||
interface FeatureGroupProps extends React.PropsWithChildren { | ||
title: string | ||
getStartedLink?: string // navLink key | ||
getStartedLink?: | ||
| keyof ReturnType<typeof useNavLinks> |
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.
do we need that first |
?
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.
Nope, it's optional - it's just for ease of readability
@@ -32,7 +32,7 @@ afterAll(() => { | |||
|
|||
const wrapper = |
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.
nit: We usually type these as React.FC<React.PropsWIthChildren>
@@ -36,12 +36,12 @@ export function useFlags(fallback) { | |||
'Warning! Self hosted build is missing a default feature flag value.' | |||
) | |||
} | |||
return fallback | |||
return fallback ?? {} |
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.
nit: could set this to the initial param as well since fallback is optional to begin with
Description
Closes codecov/engineering-team#2749
Notable Changes
Main change was for AppLink component. The props it takes in and the split of the types of components it renders wasn't ideal for translation as we render a
<Component>...</Component>
where Component could be ana
orNavLink
orLink
. It was a clever way to reuse code but I think it is worth consideration that we split these out for clarity, especially for parameter type specificity. Currently, all 3 instances receive the same props even though not all of them are needed for each type of component rendered.Screenshots
Link to Sample Entry
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.