-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix: rework overview pane in schema browser #954
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