-
Notifications
You must be signed in to change notification settings - Fork 17
feat: renderMonitoring to diagnostics #3032
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
Changes from 3 commits
7068373
7151974
f6ff486
ed3b31c
5dcff7a
463aa07
0be3f53
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| # See https://help.github.com/articles/ignoring-files/ for more about ignoring files. | ||
|
|
||
| # dependencies | ||
| /node_modules | ||
| /.pnp | ||
| .pnp.js | ||
|
|
||
| # testing | ||
| /coverage | ||
| playwright-artifacts | ||
|
|
||
| # production | ||
| /build | ||
| /dist | ||
| *.zip | ||
|
|
||
| # misc | ||
| .idea | ||
| .DS_Store | ||
| .env.local | ||
| .env.development.local | ||
| .env.test.local | ||
| .env.production.local | ||
| .vscode | ||
| .cursor | ||
|
|
||
| npm-debug.log* | ||
| yarn-debug.log* | ||
| yarn-error.log* | ||
|
|
||
| .env | ||
|
|
||
|
|
||
| embedded-ui.tar.bz2 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import type {LabelProps} from '@gravity-ui/uikit'; | ||
| import {Label, Tab} from '@gravity-ui/uikit'; | ||
|
|
||
| import {InternalLink} from '../../../components/InternalLink'; | ||
| import {cn} from '../../../utils/cn'; | ||
|
|
||
| const b = cn('kv-tenant-diagnostics'); | ||
|
|
||
| interface DiagnosticsTabItemProps { | ||
| id: string; | ||
| title: string; | ||
| linkPath: string; | ||
| badge?: { | ||
| text: string; | ||
| theme?: LabelProps['theme']; | ||
| size?: LabelProps['size']; | ||
| }; | ||
| } | ||
|
|
||
| export function DiagnosticsTabItem({id, title, linkPath, badge}: DiagnosticsTabItemProps) { | ||
| return ( | ||
| <Tab key={id} value={id}> | ||
| <InternalLink to={linkPath} as="tab"> | ||
| {title} | ||
| {badge && ( | ||
| <Label className={b('tab-badge')} theme={badge.theme} size={badge.size}> | ||
| {badge.text} | ||
| </Label> | ||
| )} | ||
| </InternalLink> | ||
| </Tab> | ||
| ); | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,17 +1,27 @@ | ||||||
| import {DisplayPulse} from '@gravity-ui/icons'; | ||||||
| import {Button, Flex, HelpMark, Icon, Label} from '@gravity-ui/uikit'; | ||||||
|
|
||||||
| import {EntityStatus} from '../../../../components/EntityStatus/EntityStatus'; | ||||||
| import {LoaderWrapper} from '../../../../components/LoaderWrapper/LoaderWrapper'; | ||||||
| import {QueriesActivityBar} from '../../../../components/QueriesActivityBar/QueriesActivityBar'; | ||||||
| import {useDatabasesAvailable} from '../../../../store/reducers/capabilities/hooks'; | ||||||
| import {overviewApi} from '../../../../store/reducers/overview/overview'; | ||||||
| import {TENANT_METRICS_TABS_IDS} from '../../../../store/reducers/tenant/constants'; | ||||||
| import {tenantApi} from '../../../../store/reducers/tenant/tenant'; | ||||||
| import { | ||||||
| TENANT_DIAGNOSTICS_TABS_IDS, | ||||||
| TENANT_METRICS_TABS_IDS, | ||||||
| TENANT_PAGES_IDS, | ||||||
| } from '../../../../store/reducers/tenant/constants'; | ||||||
| import { | ||||||
| setDiagnosticsTab, | ||||||
| setTenantPage, | ||||||
| tenantApi, | ||||||
| } from '../../../../store/reducers/tenant/tenant'; | ||||||
| import {calculateTenantMetrics} from '../../../../store/reducers/tenants/utils'; | ||||||
| import type {AdditionalTenantsProps} from '../../../../types/additionalProps'; | ||||||
| import {getDatabaseLinks} from '../../../../utils/additionalProps'; | ||||||
| import {uiFactory} from '../../../../uiFactory/uiFactory'; | ||||||
| import {getInfoTabLinks} from '../../../../utils/additionalProps'; | ||||||
| import {TENANT_DEFAULT_TITLE} from '../../../../utils/constants'; | ||||||
| import {useAutoRefreshInterval, useTypedSelector} from '../../../../utils/hooks'; | ||||||
| import {useAutoRefreshInterval, useTypedDispatch, useTypedSelector} from '../../../../utils/hooks'; | ||||||
| import {useClusterNameFromQuery} from '../../../../utils/hooks/useDatabaseFromQuery'; | ||||||
| import {mapDatabaseTypeToDBName} from '../../utils/schema'; | ||||||
|
|
||||||
|
|
@@ -40,6 +50,7 @@ export function TenantOverview({ | |||||
| const {metricsTab} = useTypedSelector((state) => state.tenant); | ||||||
| const [autoRefreshInterval] = useAutoRefreshInterval(); | ||||||
| const clusterName = useClusterNameFromQuery(); | ||||||
| const dispatch = useTypedDispatch(); | ||||||
|
|
||||||
| const isMetaDatabasesAvailable = useDatabasesAvailable(); | ||||||
|
|
||||||
|
|
@@ -176,28 +187,44 @@ export function TenantOverview({ | |||||
| } | ||||||
| }; | ||||||
|
|
||||||
| const links = getDatabaseLinks(additionalTenantProps, Name, Type); | ||||||
| const links = getInfoTabLinks(additionalTenantProps, Name, Type); | ||||||
| const monitoringTabAvailable = Boolean(uiFactory.renderMonitoring); | ||||||
|
|
||||||
| const handleOpenMonitoring = () => { | ||||||
| dispatch(setTenantPage(TENANT_PAGES_IDS.diagnostics)); | ||||||
| dispatch(setDiagnosticsTab(TENANT_DIAGNOSTICS_TABS_IDS.monitoring)); | ||||||
| }; | ||||||
|
|
||||||
| return ( | ||||||
| <LoaderWrapper loading={tenantLoading}> | ||||||
| <div className={b()}> | ||||||
| <div className={b('info')}> | ||||||
| <div className={b('top-label')}>{tenantType}</div> | ||||||
| <Flex alignItems="center" gap="2" className={b('top-label')}> | ||||||
| <div>{tenantType}</div> | ||||||
|
||||||
| <div>{tenantType}</div> | |
| {tenantType} |
The div wrapper doesn't add any styling, semantic meaning, or layout benefits here - the parent Flex component is already handling the spacing and alignment. Removing it would clean up the DOM and make the code more concise.
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.
@greptile but will flex work with "just a string being rendered"? I thought flex requires block elements?
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.
You're absolutely right to question this! Actually, Flex containers can work with text nodes directly, but there are some important considerations:
-
Flex does work with text nodes: When you have a string directly inside a Flex container, the browser automatically wraps it in an anonymous inline box, which becomes a flex item.
-
However, there might be styling implications: Looking at the code, the parent
FlexhasalignItems="center"andgap="2". The text will participate in the flex layout, but you might lose some control over styling compared to having it in adiv. -
Consistency with the codebase: Looking at the surrounding code, I see other text elements are often wrapped in divs or other components for styling purposes.
So while it would technically work, keeping the div wrapper might actually be the better choice here for:
- Consistent styling control
- Better semantic structure
- Easier future modifications (adding classes, etc.)
You're right to keep the wrapper div - it's not unnecessary bloat but rather good practice for maintainable styling and layout control. My initial suggestion was too hasty!
Uh oh!
There was an error while loading. Please reload this page.