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

Make ddg nodes circular and closer together #463

Merged
merged 8 commits into from
Oct 11, 2019

Conversation

everett980
Copy link
Collaborator

@everett980 everett980 commented Oct 10, 2019

Which problem is this PR solving?

  • DDG Nodes are fairly spread out, making it impossible to view many nodes at once while keeping text size legible.
  • Additionally, rectangular nodes do not compliment future plans for node decoration.

Short description of the changes

  • Make node spacing denser.
  • Make nodes the smallest possible enclosing circle, instead of rectangles of fixed height.

Before:

image

After

image

Unfortunately blanking out the text fails to capture how much easier it is to read the nodes, and since the font isn't mono spaced it's hard to replace with stand in text.

@yurishkuro
Copy link
Member

can you post screenshots?

@codecov
Copy link

codecov bot commented Oct 10, 2019

Codecov Report

Merging #463 into master will increase coverage by 0.24%.
The diff coverage is 94.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #463      +/-   ##
==========================================
+ Coverage   89.07%   89.31%   +0.24%     
==========================================
  Files         187      189       +2     
  Lines        4331     4410      +79     
  Branches     1045     1057      +12     
==========================================
+ Hits         3858     3939      +81     
+ Misses        427      425       -2     
  Partials       46       46
Impacted Files Coverage Δ
...epDependencies/Graph/DdgNodeContent/node-icons.tsx 100% <ø> (ø)
...ui/src/components/DeepDependencies/Graph/index.tsx 66.66% <ø> (ø) ⬆️
...onents/DeepDependencies/Graph/getNodeRenderers.tsx 14.28% <0%> (+1.78%) ⬆️
...ndencies/Graph/DdgNodeContent/calc-positioning.tsx 100% <100%> (ø)
...eepDependencies/Graph/DdgNodeContent/constants.tsx 100% <100%> (ø)
.../jaeger-ui/src/components/common/BreakableText.tsx 83.33% <100%> (ø) ⬆️
...ts/DeepDependencies/Graph/DdgNodeContent/index.tsx 38.7% <83.33%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c86466f...b9eeea6. Read the comment docs.

@everett980
Copy link
Collaborator Author

Good point, updated with sanitized screenshots and sent you two with text.

@yurishkuro
Copy link
Member

yeah, maybe replace service names with xxxx of appropriate length.

Also, it seems like the node radius changes based on the length of the service name. I think this will be pretty confusing, as it is pretty common visualization technique to vary node side based on some metric rather than the name length. I would rather let the long names bleed over, or move them below the circles.

@everett980
Copy link
Collaborator Author

Overflowing text will probably be problematic if we want to decorate nodes with a partially full band on the outside of the node.
Alternatively, we could size down the text (or truncate it with an ellipsis and a tool tip) if it makes the node too large.

@everett980
Copy link
Collaborator Author

scaling the nodes to the same size should be pretty straight forward, and will allow typical length strings to be represented legibly.

Copy link

@andreynkravtsov andreynkravtsov left a comment

Choose a reason for hiding this comment

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

Awesome work, great stuff

@everett980 everett980 merged commit 88374c6 into jaegertracing:master Oct 11, 2019
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
…odes-2

Make ddg nodes circular and closer together
Signed-off-by: vvvprabhakar <[email protected]>
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.

4 participants