-
Notifications
You must be signed in to change notification settings - Fork 405
RI-7106: additiona data in config databases open database #4539
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
RI-7106: additiona data in config databases open database #4539
Conversation
…nfo to InstancesList.tsx goToInstance
…in-CONFIG_DATABASES_OPEN_DATABASE
$isActive={isActive} | ||
$isDisabled={isDisabled} | ||
$color={color} | ||
onClick={onClick} |
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.
why this is deleted?
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.
because if it is left, it registers twice
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.
Doesn't the itemContent have onclick only in the if statement? What else is triggering the same onclick?
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.
From what I saw, there are 2 inner components based on the props:
- if disabled = true and there is an onClick handler, it renders a button
- else - renders a span
Probably this is the reason that there were 2 invocations on the handler. The tricky part is the if (isDisabled || onClick) {
condition, but if it's disabled, we probably don't want to invoke the handler, so we're safe. I'd guard it with a test, tho.
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.
If there is on click handler provided, the button that handles it is rendered, if no onClick is provided, nothing needs to handle it
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.
I can see changes on server side but there is no aprropriate tests modified/added
Approve to not block but please address this after merging please
redisinsight/api/src/modules/database/providers/database-info.provider.ts
Outdated
Show resolved
Hide resolved
description: 'Various Redis stats', | ||
type: Object, | ||
}) | ||
stats?: any; |
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.
why any
? does it make sense to have proper class for it and mapping will be done by class-transformer?
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.
Because server is using the same!?
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.
stats
is not used for anything else besides sending it for 1 telemetry event, and server
is, so if we're to type it explicitly, we should type server
explicitly as well
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.
we have 400+ any
s in our code base. It might be worthwile to type them all at some point
17298ba
Add data to:
CONFIG_DATABASES_OPEN_DATABASE
Data fields added: