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.

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

Move up redux from SpanTreeOffset #529

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aocenas
Copy link

@aocenas aocenas commented Feb 21, 2020

Which problem is this PR solving?

Short description of the changes

  • Push redux parts from SpanTreeOffset up to TracePage

This based of #517 but I cannot create a PR with that target as that branch is on my fork so this is draft until that is merged.

@codecov
Copy link

codecov bot commented Feb 21, 2020

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (8cb13c4) 92.92% compared to head (dee4680) 96.57%.
Report is 768 commits behind head on main.

❗ Current head dee4680 differs from pull request most recent head 56b37bf. Consider uploading reports for the commit 56b37bf to get more accurate results

Files Patch % Lines
.../jaeger-ui/src/components/QualityMetrics/index.tsx 93.87% 3 Missing ⚠️
...r-ui/src/components/Monitor/ServicesView/index.tsx 98.37% 2 Missing ⚠️
...components/SearchTracePage/SearchResults/index.tsx 90.00% 2 Missing ⚠️
...aeger-ui/src/actions/path-agnostic-decorations.tsx 97.91% 1 Missing ⚠️
packages/jaeger-ui/src/components/App/TopNav.tsx 94.44% 1 Missing ⚠️
...rc/components/DeepDependencies/SidePanel/index.tsx 96.77% 1 Missing ⚠️
...jaeger-ui/src/components/DependencyGraph/index.jsx 95.83% 1 Missing ⚠️
...ger-ui/src/components/Monitor/EmptyState/index.tsx 92.85% 1 Missing ⚠️
...c/components/Monitor/ServicesView/serviceGraph.tsx 98.18% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #529      +/-   ##
==========================================
+ Coverage   92.92%   96.57%   +3.64%     
==========================================
  Files         197      254      +57     
  Lines        4808     7620    +2812     
  Branches     1160     1986     +826     
==========================================
+ Hits         4468     7359    +2891     
+ Misses        299      261      -38     
+ Partials       41        0      -41     

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

@@ -19,11 +19,6 @@ jest.mock('./conv-raven-to-ga', () => () => ({
message: 'jaeger/a',
}));

jest.mock('./index', () => {
Copy link
Author

Choose a reason for hiding this comment

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

This is unrelated but did not find a way around it. The error this produces is jestjs/jest#2567 because of the process reference. I assume it could be caused be some dependency update but there is no lock file change here and I tried rm -rf node_modules and reinstalls multiple times without luck. Anybody knows why this could be happening?

@everett980
Copy link
Collaborator

everett980 commented Feb 25, 2020

Regardless of the verdict on keeping span row visibility state in redux / some parent component (discussed here), I think this state can be hidden away in TraceTimelineViewer's state. There's no need to preserve which span rows are hovered on mount/unmount as the user's cursor should be controlling this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants