-
Notifications
You must be signed in to change notification settings - Fork 527
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
[DDG stacked - 3] MouseOver UX for DDG graphs #439
Conversation
Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Joe Farro <[email protected]>
Signed-off-by: Joe Farro <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #439 +/- ##
==========================================
- Coverage 90.3% 89.04% -1.26%
==========================================
Files 180 183 +3
Lines 4145 4227 +82
Branches 1002 1020 +18
==========================================
+ Hits 3743 3764 +21
- Misses 357 417 +60
- Partials 45 46 +1
Continue to review full report at Codecov.
|
Signed-off-by: Joe Farro <[email protected]>
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.
I think there are some opportunities for clean up / clarity / naming (the hardest programming challenge) but big-picture looks great.
packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent.tsx
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DeepDependencies/Graph/index.tsx
Outdated
Show resolved
Hide resolved
packages/jaeger-ui/src/components/DeepDependencies/Graph/getNodeRenderers.css
Show resolved
Hide resolved
|
||
import { PathElem, EDdgDensity, TDdgDistanceToPathElems, TDdgModel, TDdgVertex } from './types'; |
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.
I believe this is inconsistent with the style guide wrt import order
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.
Can style issues like this be caught with an eslint rule?
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.
i don't believe linters examine the contents of imports as that could be expensive. I'll look into it as the paradigm of separating types from contents isn't rare but a quick look suggests that it's not supported.
Signed-off-by: Everett Ross <[email protected]>
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.
LGTM now. Changes ended up being fairly minor (updated some names, styling consistency, and not-yet-linted stylistic patterns).
[DDG stacked - 3] MouseOver UX for DDG graphs Signed-off-by: vvvprabhakar <[email protected]>
Which problem is this PR solving?
In DDG graphs, it's not usually possible to discern which nodes share path membership.
Short description of the changes
Add a hover effect that highlights nodes and edges that share path membership with the node being hovered.
Changes in this PR: cde52bb ... 5cd94bd
Before mouse over:
With mouse over:
Before mouse over:
With mouse over: