Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[TimelineRow]: Replace defaultProps with destructuring #2622

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

[TimelineRow]: Replace defaultProps with destructuring #2622

wants to merge 5 commits into from

Conversation

its-me-abhishek
Copy link
Contributor

@its-me-abhishek its-me-abhishek commented Jan 24, 2025 •
edited
Loading

Which problem is this PR solving?

Description of the changes

  • Remove defaultProps from the TimelineRow component, replace with destructuring

How was this change tested?

  • Run npm run test npm run update-snapshots

Checklist

@its-me-abhishek
[TimelineRow]: Replace defaultProps with destructuring
25ac5de 
Partially Resolves jaegertracing#2596
- Remove defaultProps from the TimelineRow component, replace with destructuring

Signed-off-by: Abhishek <[email protected]>
@its-me-abhishek its-me-abhishek requested a review from a team as a code owner January 24, 2025 11:02
@its-me-abhishek its-me-abhishek requested review from pavolloffay and removed request for a team January 24, 2025 11:02
@codecov Codecov
Copy link

codecov bot commented Jan 24, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 71.42857% with 2 lines in your changes missing coverage. Please review.

Project coverage is 96.59%. Comparing base (dc642b0) to head (5dacc11).

Files with missing lines Patch % Lines
...ents/TracePage/TraceTimelineViewer/TimelineRow.tsx 71.42% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2622      +/-   ##
==========================================
- Coverage   96.62%   96.59%   -0.03%     
==========================================
  Files         255      255              
  Lines        7726     7730       +4     
  Branches     1939     1994      +55     
==========================================
+ Hits         7465     7467       +2     
- Misses        261      263       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@its-me-abhishek
fix: lint issues related to onClick
95d5332 
Partially Resolves jaegertracing#2596

Signed-off-by: Abhishek <[email protected]>
yurishkuro
yurishkuro reviewed Jan 25, 2025
View reviewed changes
packages/jaeger-ui/src/components/TracePage/TraceTimelineViewer/TimelineRow.tsx Outdated
width,
style = {},
onClick,
...rest
Copy link
Member

@yurishkuro yurishkuro Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2536 (comment)

@its-me-abhishek
Copy link
Contributor Author

I have removed the ...rest from both components now, the issue has been fixed too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yurishkuro yurishkuro yurishkuro left review comments

@pavolloffay pavolloffay Awaiting requested review from pavolloffay pavolloffay is a code owner automatically assigned from jaegertracing/jaeger-maintainers

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@its-me-abhishek @yurishkuro