[TimelineRow]: Replace defaultProps with destructuring #2622
Conversation
Partially Resolves jaegertracing#2596 - Remove defaultProps from the TimelineRow component, replace with destructuring Signed-off-by: Abhishek <[email protected]>
its-me-abhishek
requested review from
pavolloffay
and removed request for
a team
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2622 +/- ##
=======================================
Coverage 96.62% 96.62%
=======================================
Files 256 256
Lines 7731 7734 +3
Branches 1942 1945 +3
=======================================
+ Hits 7470 7473 +3
Misses 261 261 ☔ View full report in Codecov by Sentry. |
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
| width, | ||
| style = {}, | ||
| onClick, | ||
| ...rest |
There was a problem hiding this comment.
I don't follow this. What can be possibly provided via ...rest if all the fields from the type are already mentioned as arguments? E.g. <TimelineRowCell data-testid="x"> should be an error because data-testid is not part of the type declaration.
There was a problem hiding this comment.
data-testid does not give error as of now, but does give an error when ...rest is removed. shall I replace ...rest with data-testid?
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
|
I have removed the ...rest from both components now, the issue has been fixed too |
yurishkuro
added
the
changelog:bugfix-or-minor-feature
| interface ITimelineRowCellProps extends React.HTMLAttributes<HTMLDivElement> { | ||
| function TimelineRowCell({ | ||
| children, | ||
| className = '', |
There was a problem hiding this comment.
I see code coverage complaining about this line. Maybe since it's already declared optional it doesn't need to have a default, and in the code use ${className || ''}?
There was a problem hiding this comment.
makes sense. I have updated it now
Partially Resolves jaegertracing#2596 Signed-off-by: Abhishek <[email protected]>
Which problem is this PR solving?
Description of the changes
How was this change tested?
npm run testnpm run update-snapshotsChecklist
jaeger:make lint testjaeger-ui:npm run lintandnpm run test