-
Notifications
You must be signed in to change notification settings - Fork 298
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-128457 / 24.10 / Reorganizing enclosure pages #10064
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10064 +/- ##
==========================================
+ Coverage 73.84% 73.88% +0.03%
==========================================
Files 1526 1529 +3
Lines 53428 53484 +56
Branches 6387 6390 +3
==========================================
+ Hits 39452 39514 +62
+ Misses 13976 13970 -6 ☔ 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.
Architecture looks good. A couple issues in code pointed out. Also, when elements select changes value, the "Disks" option stays highlighted.
] as const; | ||
|
||
return supportedViews | ||
.filter((view) => this.enclosure().elements[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.
Should declare a constant at the start and save the value and reuse it so it's clear this is a dependency for the computed signal.
component: EnclosureViewComponent, | ||
data: { title: T('View Enclosure'), breadcrumb: null }, | ||
}, | ||
{ |
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.
duplicate
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.
Please address the above suggestions. Otherwise looks good.
Doesn't seem like there is an easy solution to item highlighting in the menu. |
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.
# Conflicts: # src/assets/i18n/fr.json # src/assets/i18n/nl.json # src/assets/i18n/zh-hans.json
This PR has been merged and conversations have been locked. |
Reorganizes enclosure pages using router into different views, which can be seen by clicking in Elements menu.