-
Notifications
You must be signed in to change notification settings - Fork 17
fix: rework overview pane in schema browser #954
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
| /> | ||
| ), | ||
| }; | ||
| if (!currentSchemaData) return undefined; |
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 add curly braces
| if (isNumeric(CreateStep) && Number(CreateStep)) { | ||
| overview.push({ | ||
| label: 'Created', | ||
| value: dateTimeParse(Number(CreateStep))?.format('YYYY-MM-DD HH:mm'), |
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.
let's add helper in src/utils to format dates in one place. This helper will be used here and in formatDateTime function.
|
|
||
| let component = | ||
| currentSchemaData?.PathType && pathTypeToComponent[currentSchemaData.PathType]?.(); | ||
| // show SubType if it's not EPathSubTypeEmpty |
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.
lets remove obvious comments
| const overview: InfoViewerItem[] = []; | ||
|
|
||
| // for any schema type | ||
| overview.push({label: 'Type', value: PathType?.replace(/^EPathType/, '')}); |
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 add all labels with i18n
| if (PathType === EPathType.EPathTypeExtSubDomain) { | ||
| overview.push({ | ||
| label: 'Paths count', | ||
| value: PathDescription?.DomainDescription?.PathsInside?.length, |
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.
in the types PathsInside is a string, it seems that counting it's length is incorrect
| }); | ||
| overview.push({ | ||
| label: 'Shards count', | ||
| value: PathDescription?.DomainDescription?.ShardsInside?.length, |
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.
in the types ShardsInside is a string, it seems that counting it's length is incorrect
|
|
||
| const value = pqGroup?.PQTabletConfig?.PartitionConfig?.LifetimeSeconds; | ||
| if (value) { | ||
| const hours = (value / HOUR_IN_SECONDS).toFixed(2); |
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.
let's move this calculation to function
|
|
||
| // verbose mapping to guarantee a correct render for new path types | ||
| // TS will error when a new type is added but not mapped here | ||
| const pathTypeToComponent: Record<EPathType, (() => React.ReactNode) | undefined> = { |
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.
What do you think about not to delete this mapping? I think it's rather useful when add a new type.
In the past it was a headache to remember all places where we need to add info about new 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.
For old version we are creating a new component for any type separately. But I've rewrote the logic to have common info and adding addition info for certain type.
May be it is possible to rewrite with pathTypeToInfo function with Record<>, but I don't understand the meaning of this
I have deleted unnecessary components, because move this logic inside main component