-
Notifications
You must be signed in to change notification settings - Fork 14
feat(Healthcheck): redesign #2348
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
Conversation
bae0560
to
9136f5f
Compare
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.
Pull Request Overview
This PR redesigns the Healthcheck feature by replacing the old DiagnosticCard-based preview with a new Alert-based component and cleans up related styles and imports. It also introduces an xs
size for EmptyState, fixes import paths for EmptyFilter, adds a showVeil option to Drawer, wraps the fullscreen button in a tooltip, and adds a new HealthcheckStatus component.
Reviewed Changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/IssuesViewer/IssueTree.scss | Removed legacy issue-tree styles |
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.tsx | Refactored preview component to use Alert and new layout logic |
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.scss | Added styles for the redesigned HealthcheckPreview |
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckDetails.tsx | Deleted old details view component |
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/Healthcheck.scss | Removed legacy Healthcheck styles |
src/containers/Storage/PaginatedStorageNodesTable/StorageNodesEmptyDataMessage.tsx | Corrected EmptyFilter import path |
src/containers/Storage/PaginatedStorageGroupsTable/StorageGroupsEmptyDataMessage.tsx | Corrected EmptyFilter import path |
src/components/SplitPane/SplitPane.scss | Removed gutter z-index override |
src/components/HealthcheckStatus/i18n/index.ts & en.json & HealthcheckStatus.tsx | Added new HealthcheckStatus component and translations |
src/components/EnableFullscreenButton/EnableFullscreenButton.tsx | Wrapped fullscreen button in ActionTooltip |
src/components/EmptyState/EmptyState.tsx | Introduced xs size variant |
src/components/EmptyState/EmptyState.scss | Added styles for the new xs size |
src/components/EmptyFilter/i18n/index.ts | Fixed i18n import path |
src/components/EmptyFilter/EmptyFilter.tsx | Updated component import paths |
src/components/Drawer/Drawer.tsx | Added showVeil prop to toggle veil visibility |
src/components/Drawer/Drawer.scss | Increased drawer item z-index |
Comments suppressed due to low confidence (2)
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.tsx:23
- [nitpick] The
active
prop is declared inHealthcheckPreviewProps
but never used in the component. Remove it to clean up unused code.
active?: boolean;
src/components/SplitPane/SplitPane.scss:25
- [nitpick] Removing the gutter's
z-index
may cause it to drop behind other elements and break drag functionality. Confirm this change or add a comment explaining the new stacking context.
z-index: 10;
src/containers/Tenant/Diagnostics/TenantOverview/Healthcheck/HealthcheckPreview.tsx
Show resolved
Hide resolved
@astandrik @artemmufazalov design review passed, stand redeployed! |
src/components/Drawer/Drawer.scss
Outdated
@@ -8,7 +8,7 @@ | |||
} | |||
|
|||
&__item { | |||
z-index: 4; | |||
z-index: 11; |
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.
maybe we could make some global constants?
It unobvious what 11 here and not 12 or 10
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.
Indeed there's no need in this change. I removed it.
src/components/Drawer/Drawer.tsx
Outdated
@@ -32,6 +32,7 @@ interface DrawerPaneContentWrapperProps { | |||
detectClickOutside?: boolean; | |||
defaultWidth?: number; | |||
isPercentageWidth?: boolean; | |||
showVeil?: boolean; |
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.
maybe hideVeil with hideVeil=false is better
why? To represent the original components' prop that is "hideVeil"
width: 321px; | ||
height: 100px; | ||
#{$block}__image { | ||
margin-right: 20px; |
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.
--g-spacing-5
<Button onClick={onEnableFullscreen} view={view} disabled={disabled} title="Fullscreen"> | ||
<Icon data={SquareDashed} /> | ||
</Button> | ||
<ActionTooltip title="Fullscreen"> |
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.
i18n
[SelfCheckResult.DEGRADED]: 'info', | ||
[SelfCheckResult.MAINTENANCE_REQUIRED]: 'warning', | ||
[SelfCheckResult.EMERGENCY]: 'danger', | ||
[SelfCheckResult.UNSPECIFIED]: 'normal', |
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.
i18n
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.
it's map SelfCheckResultToLabelTheme, no need to translate here
"status_message.ok": "No issues", | ||
"no-data": "no healthcheck data" | ||
"description_problems": [ | ||
"There is {{count}} issues.", |
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.
"issues" is plural and "is" - for singular
src/uiFactory/types.ts
Outdated
@@ -10,6 +16,12 @@ export interface UIFactory { | |||
getLogsLink?: GetLogsLink; | |||
getMonitoringLink?: GetMonitoringLink; | |||
getMonitoringClusterLink?: GetMonitoringClusterLink; | |||
|
|||
getHealthckechViewTitles: GetHealthcheckViewTitles<CommonIssueType>; |
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.
maybe it would be better to make one field for all healthcheck functions? I doubt that there are two app with same getHealthckechViewTitles but different getHealthcheckViewsOrder
() => | ||
view | ||
? filteredIssues.filter((issue) => { | ||
const type = issue.upperType || issue.type; |
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.
whats upperType? I would suppose that it's issue in upperCase but looking at getLeaves function more likely its parentType
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 structure is: Root => firstParent => ... => nthParent => child.
UpperType is the type of the firstParent... I will reword.
export function Issues({issues}: IssuesProps) { | ||
const {view, issuesFilter} = useTenantQueryParams(); | ||
|
||
const filteredIssues = React.useMemo(() => { |
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.
maybe could try to hide them with css has-text?
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.
unfortunately not possible for now
return null; | ||
} | ||
|
||
return pdisk.map((el: {id: string; path: string}) => ( |
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.
maybe something instead of el would be great =)
maybe when searching - we could open found issues? |
@@ -35,6 +35,7 @@ Props description: | |||
6) inverseColorize - invert the colors of the progress bar | |||
7) warningThreshold - the percentage of fullness at which the color of the progress bar changes to yellow | |||
8) dangerThreshold - the percentage of fullness at which the color of the progress bar changes to red | |||
9) overflow - parcents may be more that 100% | |||
*/ |
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.
pErcents
position: sticky; | ||
z-index: 1; | ||
top: 0px; | ||
left: 0; |
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.
top, left lets both be 0px or 0
closes #2308
Stand
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
😟 No changes in tests. 😕
Bundle Size: 🔺
Current: 83.76 MB | Main: 83.65 MB
Diff: +0.10 MB (0.12%)
ℹ️ CI Information