[AccordianKeyValues]: replace defaultProps with destructuring #2667
Conversation
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
its-me-abhishek
requested review from
jkowall
and removed request for
a team
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2667 +/- ##
==========================================
- Coverage 96.62% 96.59% -0.03%
==========================================
Files 256 256
Lines 7733 7733
Branches 1946 2022 +76
==========================================
- Hits 7472 7470 -2
- Misses 261 263 +2 ☔ View full report in Codecov by Sentry. |
yurishkuro
added
the
changelog:bugfix-or-minor-feature
| // export for tests | ||
| export function KeyValuesSummary(props: { data?: KeyValuePair[] }) { | ||
| const { data } = props; | ||
| const { data } = props || null; |
There was a problem hiding this comment.
don't follow this. If props are empty you replace them with null? How then can you deconstruct null into { data } object?
There was a problem hiding this comment.
i think setting this to
const { data = null } = props || {};
should work
if props is truthy, destructuring happens normally else if props is falsy (null, undefined, false, etc.), then {} is used instead as null value
There was a problem hiding this comment.
props || {}
why do you need even that? props are not nullable in the signature, so they are already required to be at least {}
| linksGetter: ((pairs: KeyValuePair[], index: number) => Link[]) | TNil; | ||
| onToggle?: null | (() => void); | ||
| }; | ||
|
|
||
| // export for tests | ||
| export function KeyValuesSummary(props: { data?: KeyValuePair[] }) { |
There was a problem hiding this comment.
KeyValuesSummary is used from AccordianKeyValues, where data is not nullable. Why suddenly does it become nullable here?
There was a problem hiding this comment.
I had set this as the defaultProps suggested for KeyValuesSummary that the data would be null by default
KeyValuesSummary.defaultProps = {
data: null,
};
There was a problem hiding this comment.
doesn't answer my question. Previous bug is not the reason to repeat it.
There was a problem hiding this comment.
was unaware of that, in that case I guess
export function KeyValuesSummary({ data }: { data: KeyValuePair[] }) {
works well here, and also passes the CI, as only data was being passed through props so I have replaced it, and removed the nullable assignment
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
|
can you add tests to in crease coverage? |
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
|
please run |
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
…ek/jaeger-ui into accordiankeyvalues
|
still shows as uncovered, will try to find a fix for this |
Which problem is this PR solving?
Description of the changes
How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test