-
Notifications
You must be signed in to change notification settings - Fork 389
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
CB-4652 migrate server configuration form #2916
Conversation
export type IServerConfigurationFormPartState = { | ||
serverConfig: schema.infer<typeof ServerConfigurationFormPartStateConfigSchema>; | ||
navigatorConfig: schema.infer<typeof ServerConfigurationFormPartStateNavigatorSchema>; | ||
}; |
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 not ?
const ServerConfigurationFormPartStateSchema = schema.object({
serverConfig: ServerConfigurationFormPartStateConfigSchema,
navigatorConfig: ServerConfigurationFormPartStateNavigatorSchema
});
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.
Seems like we cant use ServerConfigurationFormPartStateConfigSchema as a type cause its a value
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.
Nice refactor!
await this.authProvidersResource.load(CachedMapAllKey); | ||
|
||
if (this.authProvidersResource.has(AUTH_PROVIDER_LOCAL_ID)) { | ||
await this.passwordPolicyResource.load(); |
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 preloaded the resource, but it is not used in this current form part. Seems like it can be removed from here completely
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.
const config = await this.serverConfigResource.load(); | ||
const productInfo = await this.productInfoResource.load(); | ||
const defaultNavigatorSettings = await this.defaultNavigatorSettingsResource.load(); |
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 load data in parallel mode so user have a better UX
const config = await this.serverConfigResource.load(); | |
const productInfo = await this.productInfoResource.load(); | |
const defaultNavigatorSettings = await this.defaultNavigatorSettingsResource.load(); | |
[config, productInfo, defaultNavigatorSettings] = await Promise.all([ | |
this.serverConfigResource.load(), | |
this.productInfoResource.load(), | |
this.defaultNavigatorSettingsResource.load() | |
]); |
function reset() { | ||
service.loadConfig(true); | ||
} | ||
const changed = part.isChanged; |
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.
currently it is a function so it is not working properly
after the merge of bad.setState ticket should be fine I suppose. but please check it after the merge wether it is working correctly
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've tested it on your branch
…ithub.com/dbeaver/cloudbeaver into CB-4652-migrate-server-configuration-form
No description provided.