-
Notifications
You must be signed in to change notification settings - Fork 311
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-132759 / 25.04 / Rework Installed Apps #11120
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## master #11120 +/- ##
==========================================
+ Coverage 82.38% 82.56% +0.18%
==========================================
Files 1648 1650 +2
Lines 57694 58008 +314
Branches 5944 5954 +10
==========================================
+ Hits 47532 47896 +364
+ Misses 10162 10112 -50 ☔ 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.
Looks good overall but can be improved.
#masterDetailView="masterDetailViewContext" | ||
[selectedItem]="selectedApp" | ||
> | ||
<ng-container master> |
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.
Would be good to rework that to have a separate component for installed-apps-list
, so the installed-apps
component html may become smaller.
As example:
<ix-page-header>
<ix-all-instances-header></ix-all-instances-header>
</ix-page-header>
<ix-master-detail-view
#masterDetailView="masterDetailViewContext"
[selectedItem]="selectedInstance()"
>
<ix-instance-list
master
[isMobileView]="masterDetailView.isMobileView()"
[showMobileDetails]="masterDetailView.showMobileDetails()"
(toggleShowMobileDetails)="masterDetailView.toggleShowMobileDetails($event)"
></ix-instance-list>
<ng-container detail-name>
{{ selectedInstance()?.name }}
</ng-container>
<ng-container detail>
@if (selectedInstance()) {
<ix-instance-details
[instance]="selectedInstance()"
></ix-instance-details>
}
</ng-container>
</ix-master-detail-view>
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.
As well would be good to extract bulk actions to the separate component
Example:
@if (checkedInstances?.length > 0) {
<ix-instance-list-bulk-actions
[checkedInstances]="checkedInstances"
(resetBulkSelection)="resetSelection()"
></ix-instance-list-bulk-actions>
}
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.
Seems to be working the same as before
This PR has been merged and conversations have been locked. |
Changes:
Used new master-detail view component on installed apps page
Testing:
Double check that all features of the installed Apps page work