dbeaver/pro#9614 add predefined param for connection type#4454
dbeaver/pro#9614 add predefined param for connection type#4454devnaumov wants to merge 8 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 76 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
|
||
| // TODO: must be loaded in place where it is used | ||
| useResource(Body, ProjectInfoResource, CachedMapAllKey, { silent: true }); | ||
| useResource(Body, ConnectionTypeResource, CachedMapAllKey, { silent: true }); |
There was a problem hiding this comment.
lets load in place where it is required cause now the tracks are hot and we can do it quite easy
from my perspective inside Connection Tree should be enough, what do you think? cause this tree like is always gonna be shown despite everything and the feature is strongly sticked to it
There was a problem hiding this comment.
also in administration part when we load connection types tab
There was a problem hiding this comment.
The connection tree might be hidden for certain roles in TE. Later we will show types for every tree that connected to the connection, for example ai chat custom scope tree and etc. Connection types are strongly connected to the whole app as projects and we want them to be ready as soon as possible. As we will not use this resource in place where we resolve type color, it will be just useResource(Component, ConnectionTypeResource, CachedMapAllKey); in like 10 places that is confusing as well.
There was a problem hiding this comment.
okay then adding it to useConnectionTypeColor.ts should be enough, I guess
There was a problem hiding this comment.
Its better to now use useResource hook inside hooks that might be used inside nodes or cells as its very perfomance heavy. In our case we use it for navigation tree
| export type NewConnectionType = ConnectionType & { [NEW_CONNECTION_TYPE_SYMBOL]: boolean; timestamp: number }; | ||
|
|
||
| @injectable(() => [GraphQLService, SessionResource]) | ||
| export class ConnectionTypeResource extends CachedMapResource<string, ConnectionType> { |
There was a problem hiding this comment.
I think we should outdate ConnectionInfoResource if this resource is outdated to keep things fresh
| export function compareNewConnectionTypes(a: ConnectionType, b: ConnectionType): number { | ||
| if (isNewConnectionType(a) && isNewConnectionType(b)) { | ||
| return b.timestamp - a.timestamp; | ||
| } | ||
|
|
||
| if (isNewConnectionType(b)) { | ||
| return 1; | ||
| } | ||
|
|
||
| if (isNewConnectionType(a)) { | ||
| return -1; | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
what about predefined flag? do we want to consider it here as well?
There was a problem hiding this comment.
This function only compares by the new flag, general sorting happens inside compare fn as we do in other resources
| export function useConnectionTypeColor(connectionKey: IConnectionInfoParams | undefined): string | undefined { | ||
| const connectionTypeService = useService(ConnectionTypeService); | ||
|
|
||
| if (!connectionKey) { | ||
| return; | ||
| } | ||
|
|
||
| return connectionTypeService.getConnectionTypeColor(connectionKey); | ||
| } |
There was a problem hiding this comment.
maybe here we also can use useResource for ConnectionInfoResource & ConnectionTypeResource to be sure all loaded and no cases when we try to calc color but it is not preloaded with resources
There was a problem hiding this comment.
Its better to now use useResource hook inside hooks that might be used inside nodes or cells as its very perfomance heavy
|
|
||
| &:not(:global([aria-selected='true'])) { | ||
| background-color: transparent !important; | ||
| background-color: transparent; |
There was a problem hiding this comment.
autotests required then to see if no visual regression
| <FolderTabRenderer key={folderId} nodeId={nodeId} folderId={folderId} parents={parents} /> | ||
| ))} | ||
| </TabList> | ||
| <div> |
There was a problem hiding this comment.
why do we need div here?
There was a problem hiding this comment.
Because we need background color to be applied around tabs and not on the tabs itself
| import type { IConnectionInfoParams } from '../CONNECTION_INFO_PARAM_SCHEMA.js'; | ||
|
|
||
| @injectable(() => [ConnectionTypeResource, ConnectionInfoResource, ThemeService]) | ||
| export class ConnectionTypeService { |
There was a problem hiding this comment.
you have 2 ways to calculate colors now:
- This
service useConnectionTypeColorhook
both has similar interface. you need a connectionKey to keep things going
both cases uses hook and the service for tsx components
seems like you can just use the hook and pass to it something like:
{connectionId?: string, projectId?: string} - and handle edge case if nothing is provided inside the hook to avoid these if/else clauses you have for service case of implementation
There was a problem hiding this comment.
so smells like we can just remove this service and move all of its logic inside the hook
There was a problem hiding this comment.
We cant, there might be cases where you need this color in some service or not in react context in general. When all your logic inside hook, you can only use it inside components. Also in some places we dont use hook (for example when you cant use it cause you cant use hooks conditionally), there we get color from service. So the service is source of truth here
| "@cloudbeaver/core-root": "workspace:*", | ||
| "@cloudbeaver/core-sdk": "workspace:*", | ||
| "@cloudbeaver/core-settings": "workspace:*", | ||
| "@cloudbeaver/core-theming": "workspace:*", |
There was a problem hiding this comment.
pls create PR for widgets repo
closes 9614