-
Notifications
You must be signed in to change notification settings - Fork 318
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
NAS-131978 / 25.04 / Instances table and layout #10971
Conversation
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 good overall.
- The controls column could use more breathing room, right now everything is very glued to the right.
- When I use search and there are no results, the empty text should be updated.
@@ -0,0 +1,17 @@ | |||
import { marker as T } from '@biesbjerg/ngx-translate-extract-marker'; | |||
|
|||
export enum DockerNvidiaStatus { |
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.
Should part of a different PR because we would need to backport it.
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.
Wrong branch. Changes were reverted
this.selectFirstApp(); | ||
} | ||
|
||
private selectFirstApp(): void { |
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.
Method needs to be renamed.
this.showMobileDetails.set(false); | ||
} | ||
|
||
restart(instanceId: string): void { |
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.
Any particular reason to do it here and not in the child component?
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.
The idea is to separate logic from ix-instance-row
using it solely for representation purposes.
<div class="container"> | ||
<div class="table-container"> | ||
<div class="table-header"> | ||
<h2>{{ 'Instances' | translate }}</h2> |
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.
Let's try without header here and in details.
40c0a7e
to
0862039
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #10971 +/- ##
==========================================
- Coverage 81.94% 81.92% -0.03%
==========================================
Files 1609 1612 +3
Lines 56692 56848 +156
Branches 5868 5881 +13
==========================================
+ Hits 46459 46573 +114
- Misses 10233 10275 +42 ☔ View full report in Codecov by Sentry. |
This PR has been merged and conversations have been locked. |
Changes:
Implement instances table and layout
Testing:
Go to Containers. Check original ticket.