Skip to content
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-128451 / 24.10 / Port WidgetSysInfoComponent to new dashboard #10047

Merged
merged 21 commits into from May 20, 2024

Conversation

denysbutenko
Copy link
Member

There are some TODOs to resolve but it is ready for review and testing.

The widget has some changes:

  • Show scale image placeholder even if it is Unsupported Hardware.

  • When a system has HA but HA is disabled the triangle exclamation icon and tooltip was moved into the button:
    image

  • When a system has running update UPDATE IN PROGRESS string was moved into the button:
    image

  • The system reports icon was added on widget for active node that brings user to /reportsdashboard/system
    image

# Conflicts:
#	src/app/pages/dashboard/dashboard.module.ts
#	src/app/pages/dashboard/services/dashboard.store.ts
#	src/app/pages/dashboard/services/widget-resources.service.ts
#	src/app/pages/dashboard/types/widget-category.enum.ts
#	src/app/pages/dashboard/widgets/all-widgets.constant.ts
#	src/assets/i18n/nl.json
#	src/assets/i18n/zh-hans.json
@denysbutenko denysbutenko requested a review from a team as a code owner May 10, 2024 13:11
@denysbutenko denysbutenko requested review from RehanY147 and removed request for a team May 10, 2024 13:11
@bugclerk bugclerk changed the title Port WidgetSysInfoComponent to new dashboard NAS-128451 / 24.10 / Port WidgetSysInfoComponent to new dashboard May 10, 2024
@bugclerk
Copy link
Contributor

@denysbutenko
Copy link
Member Author

@RehanY147 you're lucky :)

@truenas truenas deleted a comment from codecov bot May 16, 2024
Copy link

codecov bot commented May 16, 2024

Codecov Report

Attention: Patch coverage is 79.61165% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 73.95%. Comparing base (d097965) to head (874c9f7).
Report is 14 commits behind head on master.

Files Patch % Lines
...ges/dashboard/services/widget-resources.service.ts 20.00% 8 Missing ⚠️
src/app/store/system-info/system-info.effects.ts 0.00% 7 Missing ⚠️
...ts-dashboard/components/report/report.component.ts 16.66% 5 Missing ⚠️
...-info-passive/widget-sys-info-passive.component.ts 90.69% 4 Missing ⚠️
src/app/modules/jobs/store/job.selectors.ts 40.00% 3 Missing ⚠️
...ard/widgets/system/common/widget-sys-info.utils.ts 89.28% 3 Missing ⚠️
...ngs/support/support-card/support-card.component.ts 62.50% 3 Missing ⚠️
src/app/store/system-info/system-info.selectors.ts 57.14% 3 Missing ⚠️
...rc/app/pages/dashboard/services/dashboard.store.ts 0.00% 2 Missing ⚠️
...nents/widget-sys-info/widget-sys-info.component.ts 83.33% 1 Missing ⚠️
... and 3 more
Additional details and impacted files
@@             Coverage Diff             @@
##           master   #10047       +/-   ##
===========================================
+ Coverage    4.84%   73.95%   +69.10%     
===========================================
  Files        1526     1537       +11     
  Lines       53446    53675      +229     
  Branches     6387     6410       +23     
===========================================
+ Hits         2592    39696    +37104     
+ Misses      50854    13979    -36875     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@denysbutenko
Copy link
Member Author

@RehanY147 okay I made bunch of adjustments and ready for another review.

src/app/constants/time.constant.ts Outdated Show resolved Hide resolved
isHaLicensed = input.required<boolean>();
isIxHardware = input.required<boolean>();

isUnsupportedHardware = computed(() => this.isEnterprise() && !this.isIxHardware() && !this.isHaLicensed());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer writing this like the following

isUnsupportedHardware = computed(() => {
  const isEnterprise = this.isEnterprise();
  const isIxHardware = this.isIxHardware();
  const isHaLicensed = this.isHaLicensed();
  return isEnterprise && !isIxHardware && !isHaLicensed;
});

Because of the way JavaScript handles comparison operators, it can ignore a signal dependency in a computed signal if during the initial execution, the signal was never fetched. For example, computed(() => this.isEnterprise() || this.isHa()) will ignore this.isHa() every time if this.isEnterprise() is always true. So, this.isHa() is never considered a dependency for the computed signal.
This applies to all computed signals. As a best practice, any dependency signals should be fetched at the start of the compute callback function into variables so it's clear that they are all dependencies.

@denysbutenko denysbutenko enabled auto-merge (squash) May 20, 2024 06:55
@denysbutenko denysbutenko merged commit 59ccfd0 into master May 20, 2024
10 checks passed
@denysbutenko denysbutenko deleted the NAS-128451 branch May 20, 2024 07:18
@bugclerk
Copy link
Contributor

This PR has been merged and conversations have been locked.
If you would like to discuss more about this issue please use our forums or raise a Jira ticket.

@truenas truenas locked as resolved and limited conversation to collaborators May 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants