-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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: Network view should group pods into higher level workload (#5468) #8996
feat: Network view should group pods into higher level workload (#5468) #8996
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8996 +/- ##
==========================================
- Coverage 46.06% 46.04% -0.02%
==========================================
Files 217 217
Lines 25908 25912 +4
==========================================
- Hits 11934 11931 -3
- Misses 12317 12323 +6
- Partials 1657 1658 +1
Continue to review full report at Codecov.
|
Simple-TreeView.movSimple-NetworkView.mov |
Canary-TreeView.movCanary-NetworkView.mov |
CertManager-TreeView.movCertManager-NetworkView.mov |
ui/src/app/applications/components/application-resource-tree/application-resource-tree.tsx
Outdated
Show resolved
Hide resolved
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.
Looks great!! Since the grouped pod view looks very similar to the Pods view, is there a simple way to make it partially reusable component? Just curious.
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.
A couple of small things, otherwise looks good. I also agree with @ciiay 's comment about potentially factoring out the inner pod component into a shared component between this and pod view. That can perhaps be done later though.
Thanks!
ui/src/app/applications/components/application-resource-tree/application-resource-tree.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-resource-tree/application-resource-tree.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/applications/components/application-resource-tree/application-resource-tree.tsx
Outdated
Show resolved
Hide resolved
fa4e090
to
ad60d47
Compare
Hi @rbreeze , sorry about the delay. I've rebased and resolved the review comments. Please re-review/merge? |
I opened #9338 to track the two review comments to pull out the pod group into a common component so that both the Tree view and the Pods view can use. |
…proj#5468) Signed-off-by: Keith Chong <kykchong@redhat.com>
ad60d47
to
9ffbe39
Compare
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, thanks @keithchong!
Thanks @rbreeze |
Signed-off-by: Keith Chong kykchong@redhat.com
Fixes #5468. This Pod Group applies to both the Details Network view and the Tree view. The Pod Group is made available to the tree-based views so that it can be integrated with the Tree Expansion support. eg. the Pod Group can be the collapsed state for the higher level workload, and when you expand it, it will expand out the Pods.
Notes:
At a high level, the general solution involves flagging all potential Pod Groups (that is, “parents” of pods in the graph views). It piggy backs on the existing logic (loops) when gathering up the graph nodes. Once a pod is found, it can be a graph node, or not, and it depends on the property setting showCompactNodes, which I’m using to turn the feature on and off. The pod is then added to the pod group and it continues on with the next iteration of the loop.
I initially wanted to make the pod group look like the pod group from the Pods view. But it was better to try to make it look like the existing nodes in the tree and network views so that the animation transition looks smoother.
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: