-
Notifications
You must be signed in to change notification settings - Fork 65
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
[YUNIKORN-2961] Move node utilizations chart to dashboard page #224
Conversation
5fd1683
to
ff00a27
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #224 +/- ##
=======================================
Coverage 35.71% 35.71%
=======================================
Files 2 2
Lines 56 56
=======================================
Hits 20 20
Misses 33 33
Partials 3 3 ☔ View full report in Codecov by Sentry. |
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.
Mostly good, just a couple points.
.app-node-utilizations { | ||
::ng-deep .app-node-utilizations { | ||
.card-wrapper { | ||
max-width: 800px; |
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 think we should have a max-width here. With lots of resource types, the extra horizontal real estate can be nice to have.
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.
This also means the chart should expand to fill the available space.
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.
Okay. I have removed the restrict, the updated demo shown as above.
The reason why I apend the restrict is consider the right side partition selector might be far for user.
Updated:
Oh, I find the chart maintainAspectRatio setting could address the issue.
It let chart fit the container.
src/app/components/app-node-utilizations/app-node-utilizations.component.html
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.
+1 LGTM.
What is this PR for?
What type of PR is it?
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2961
Screenshots (if appropriate)
bandicam.2024-11-14.23-02-44-261.mp4
bandicam.2024-11-14.23-07-50-521.mp4
bandicam.2024-11-15.20-55-51-284.mp4