-
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-133494 / 25.04 / WIP: Enclosure Colors #11345
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
❌ Your patch check has failed because the patch coverage (61.53%) is below the target coverage (75.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #11345 +/- ##
==========================================
+ Coverage 82.92% 82.93% +0.01%
==========================================
Files 1666 1669 +3
Lines 59715 59947 +232
Branches 6276 6303 +27
==========================================
+ Hits 49516 49717 +201
- Misses 10199 10230 +31 ☔ 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.
Overall looks nice, thank you.
Let's clean up some things in code.
.../pages/system/enclosure/components/enclosure-side/enclosure-svg/enclosure-svg.component.scss
Outdated
Show resolved
Hide resolved
|
||
this.renderer.removeClass(overlay, 'tinted'); | ||
private dimSlot(slotNumber: number, opacity: number): void { | ||
const slotId: string = 'DRIVE_CAGE_' + slotNumber.toString(); |
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 would suggest updating code that initially looks for drives cages in processSvg
to store them as class properties and then using that here and below.
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.
@undsoft Updated the file. Please check again.
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.
Something is wrong now.
- Rear slots on M60 are incorrect.
- Front slots on R20 are incorrect.
I reverted the change. R20 is not on the list of updated enclosures for this PR. I only did a cross section of models for review purposes. I will update the others in a separate PR once this gets merged |
So, why not forward fix it? We still need to make code updates. |
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.
Some models, including but not limited to R20, ES24N, ES60, ES102 are broken compared to master.
Rackmount models are done. Only the Desktop MINIs should be left |
Changes:
Changes how we apply color coding to
Testing:
Use mock enclosure utility and test for models M60, ES24, ES24F, MINI-R, H-Series, X-Series, ES12, ES102G2. These models were chosen to show a variety of different layouts and drive tray types. Once this PR is approved the other models will be updated.
Downstream
This only touches the enclosure-svg component files and the SVG assets themselves. Assets are updated with some class selectors and CSS. Nothing else should be affected.
This will affect documentation as screen shots of the enclosure visualization will look different now.