-
Notifications
You must be signed in to change notification settings - Fork 176
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
fix(ui): hide subscriptions per warehouse in DAG #2396
Conversation
Signed-off-by: Remington Breeze <[email protected]>
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2396 +/- ##
=======================================
Coverage 48.38% 48.38%
=======================================
Files 245 245
Lines 17761 17761
=======================================
Hits 8593 8593
Misses 8748 8748
Partials 420 420 ☔ View full report in Codecov by Sentry. |
This still has the same problems that we encountered with #2211. When you have multiple Warehouses, no matter what hide button you click, the subscriptions are hidden from bottom to top and no matter what show button you click, the subscriptions are revealed from top to bottom. |
Signed-off-by: Remington Breeze <[email protected]>
Signed-off-by: Remington Breeze <[email protected]>
Signed-off-by: Remington Breeze <[email protected]>
@krancour and I discussed offline. I changed the mechanism by which this hiding works to solve the problem: before, the approach was to remove the subscription nodes before the DAG was calculated. Now, we render the DAG the same regardless of which subscriptions are hidden, and simply don't render the hidden nodes. Before, the DAG jumped around, and now the DAG is stable. Untitled.mov |
Thanks @rbreeze. I tried it out and it works great. The only potential issue I see is that once all the subscriptions are hidden, since they're merely invisible, they're still taking up screen real estate, which may be irksome for some longer DAGs. I think you were working on doing the DAG differently anyway and this fix is just an interim one? If that's the case, this LGTM. |
@krancour I agree, that would be an annoyance for sure. You're correct, I'm currently working on rewriting the DAG rendering to take place on the backend, so I can certainly fix this issue as part of that overhaul. |
Fixes #2125