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

feat: Grid-tree view to improve information density and preserve visual hierarchy (#6936) #8661

Merged
merged 1 commit into from May 16, 2022

Conversation

keithchong
Copy link
Contributor

@keithchong keithchong commented Mar 3, 2022

Signed-off-by: Keith Chong kykchong@redhat.com

This fixes #6936

This is a preliminary solution to address the concerns in 6936, as well as several other issues.

Points:

  1. This PR changes the line connections that are somewhat disorganized, with multiple outward-bound points from the originating node, and multiple inward-bound points to the target node. At times, it is hard to distinguish which nodes the lines are connected to. For example: https://cd.apps.argoproj.io/applications/ingress-nginx?view=network&resource=
  2. This PR adds expansion and collapse support for each node that has children. This directly addresses the information density problem. The Expand All and Collapse All buttons are added to the floating toolbar.
  3. The line connection re-layout change can be pulled out into a separate PR if desired.
  4. The implementation of the expansion and collapse feature involves keeping track of the collapsed nodes. Upon entering the tree view, by default, all nodes are expanded. Therefore, no node is in the list of collapsed nodes. Collapse All means all applicable nodes will be added to this list. Performance might be a concern here so the Collapse All button could be removed. Users might not collapse all nodes by hand anyway. Perhaps, another solution will be to write the expansion state value as an attribute on the element.
  5. It is hard to distinguish the node "kind" in the view. This fix includes changing the background color for Pods. This change can be easily removed, but is included here to address this usability issue and a separate feature can be opened to perhaps customize the node colors.
  6. Can't find Expand/Collapse All icons with multiple squares behind the +/- symbol. These are better than using only the plus and minus icons.
  7. Expansion state for a node applies to (is preserved in) BOTH the Tree view and the Network view.
  8. The issue in Network view should group pods into higher level workload #5468 is arguably in part due to the many pods that are shown. However, in honoring the concept of the tree structure, and the parent-child relationship, part of this concern is addressed. Note, this fix does not preclude the fix for 5468, since the collapsed state can still show the "Pod view node" representation.
  9. Came across some issue where part of the connector lines is not rendering when zooming out. Haven't found out why.
  10. Grouped-Nodes feature works normally.

Please try this fix out, especially with applications with many nodes.

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@codecov
Copy link

codecov bot commented Mar 3, 2022

Codecov Report

Merging #8661 (9833bbe) into master (727c59f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8661   +/-   ##
=======================================
  Coverage   45.78%   45.78%           
=======================================
  Files         220      220           
  Lines       26170    26170           
=======================================
  Hits        11981    11981           
  Misses      12531    12531           
  Partials     1658     1658           

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 727c59f...9833bbe. Read the comment docs.

@keithchong
Copy link
Contributor Author

keithchong commented Mar 3, 2022

Network View
Screenshot from 2022-03-03 02-33-57

Screenshot from 2022-03-03 02-47-34

Tree View
Screenshot from 2022-03-03 02-44-35

Collapsed ReplicaSet
Screenshot from 2022-03-03 02-44-47

@keithchong keithchong changed the title UI: Grid-tree view to improve information density and preserve visual hierarchy (#6936) feat: Grid-tree view to improve information density and preserve visual hierarchy (#6936) Mar 3, 2022
@keithchong keithchong requested a review from rbreeze March 3, 2022 15:33
@keithchong
Copy link
Contributor Author

Hey @rbreeze, could you please have a look at this?

@keithchong
Copy link
Contributor Author

Before:

Screen Shot 2022-03-03 at 10 36 00 AM

@keithchong keithchong requested a review from alexmt March 3, 2022 15:44
@keithchong keithchong force-pushed the Expansion01 branch 4 times, most recently from 097bff0 to 5bd2b8b Compare March 3, 2022 17:33
Copy link
Contributor

@reginapizza reginapizza left a comment

Choose a reason for hiding this comment

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

Hi Keith! I checked this out and it looks good to me. One suggestion I have is to add a tooltip to the smaller +/- on the resource so that a user will know it's for collapsing/expanding child nodes. You also had a point about the line connectors disappearing when zoomed out but I didn't observe that so I'm curious where you're seeing that happening?

@ciiay
Copy link
Contributor

ciiay commented Mar 4, 2022

Hi Keith, great work on this complicated task! Good learning from reviewing the PR 👍 Thank you for that.

I only have one small concern that if we can pass the color to the SCSS so that the right arrow would have same color as the dashed line in the network view? That way it looks a little bit more clearer when it comes to very complex graph.

You probably already know the trick, but here's an example of what I mean: https://css-tricks.com/getting-javascript-to-talk-to-css-and-sass/

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

Looks good, but I agree with both @reginapizza and @ciiay 's suggestions that:

  1. Matching the color of the arrows in network view and
  2. Adding a tooltip indicating what the minimize/maximize button does

@keithchong
Copy link
Contributor Author

keithchong commented May 11, 2022

The color of the right arrow is an issue without this enhancement. But, yes, it will make the graphical view more refined and polished:

MatchingColoredArrow.mov

@keithchong
Copy link
Contributor Author

keithchong commented May 11, 2022

Also, the arrow will be centred along the connecting line and the line ends at the wide part of the arrow, unlike the current behavior now:

ArrowsCentered-LinesEndAtArrow.mov

@keithchong keithchong force-pushed the Expansion01 branch 2 times, most recently from 264871c to 8628614 Compare May 15, 2022 18:47
Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

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

LGTM once comment is addressed

Signed-off-by: Keith Chong <kykchong@redhat.com>
@keithchong
Copy link
Contributor Author

More testing:

Before:

Before-NetworkView

After:

After-NetworkView

From resolved conversation, pod and non-pod children of parent:

Before:
Before-ChildPodsAndNonPods

After:

After-ChildPodsAndNonPods

After-PodGroupWithExpansionToAccountForNonPodChildren

@keithchong
Copy link
Contributor Author

One issue (one can argue it isn't an issue) is that because the lines are overlapping, the more lines there are, the more they are 'concentrated' so that the connector lines will appear more solid, as shown in the screenshot below. Notice the connector lines are dashed and appear more 'normal' when the connections are toward the outer pods (because there are fewer overlapping lines)

ManyOverlappingLineConnectors

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.

UI: Grid-tree view to improve information density and preserve visual hierarchy
4 participants