feat(workspaces): move tab rendering to the plugins COMPASS-9413 #6997
Conversation
| isNonExistent?: boolean; | ||
| iconGlyph: GlyphName | 'Logo' | 'Server'; | ||
| tooltip?: [string, string][]; | ||
| tabTheme?: Partial<TabTheme>; |
There was a problem hiding this comment.
We could get rid of this prop and instead use a provider.
Here's a branch with that provider: main...tab-theme-context-provider-example
I didn't include it in these changes, as it is something we could do as a follow up.
There was a problem hiding this comment.
Makes sense to me, and then we can also add it to all the shared workspace providers. Then we can also use these colors inside the workspaces UI easily if we need to
| let isNonExistent: boolean | undefined; | ||
| if ('isNonExistent' in tab && tab.isNonExistent) { | ||
| if (tab.type === 'Collections') { | ||
| // TODO: Can/should we move this logic into the collections plugin title? |
There was a problem hiding this comment.
I would expect it to be with the tab title rendering logic, yes
There was a problem hiding this comment.
Created a ticket for this to do as a follow up COMPASS-9456. Neither of the plugins have this in their store so it would take some more work and I'm thinking we could do that seperately.
Anemy
changed the title
Anemy
added
feature flagged
| }} | ||
| > | ||
| <WorkspaceTabContextProvider tab={tab} sectionType="tab-title"> | ||
| <Provider> |
There was a problem hiding this comment.
This should go inside the WorkspaceTabContextProvider and be added as the last wrapper over the rendered content there
There was a problem hiding this comment.
Or at least I think so, maybe I'm missing why it shouldn't 😆
| <WorkspaceTabContent | ||
| // Cast to any as it can be any of the workspace types | ||
| // here. | ||
| {...(tab as any)} |
There was a problem hiding this comment.
If you look at the logic inside the WorkspaceTabContextProvider it's actually not all the props that need to be passed down, like you can do it, but as always it's better to be a bit more explicit. If you move the Provider inside WorkspaceTabContextProvider though this should be handled for you consistently (and there are some type guarantees provided by the method that picks those out)
There was a problem hiding this comment.
Updated this to keep it all in WorkspaceTabContextProvider except for the isNonExistent which will be going away soon anyway.
| header: (props: { | ||
| tabProps: WorkspaceTabCoreProps; | ||
| workspaceProps: WorkspaceTabProps; | ||
| }) => ReturnType<typeof Tab>; |
There was a problem hiding this comment.
Because you mentioned that you're still wrangling types here, I'd maybe suggest to change this to:
| header: (props: { | |
| tabProps: WorkspaceTabCoreProps; | |
| workspaceProps: WorkspaceTabProps; | |
| }) => ReturnType<typeof Tab>; | |
| header: (props: WorkspaceTabCoreProps & WorkspaceTabProps) => ReturnType<typeof Tab>; |
That way provider can spread required ones easier I think
There was a problem hiding this comment.
I like it, simplifies things, updated!
| const { id: connectionId } = useConnectionInfo(); | ||
|
|
||
| const { database, collection, ns } = toNS(namespace); | ||
| const connectionName = getConnectionById(connectionId)?.title || ''; |
There was a problem hiding this comment.
There's a small issue with useConnectionInfo implementation (and so the types are correctly hiding that title is already returned here due to the test value fallback), but you can clean this up a bit and just access title from connectionInfo returned by the hook (but feel free to keep this out of this PR to avoid more changes piling up)
There was a problem hiding this comment.
Good callout. Tests are green now, I'll do that in a follow up!
COMPASS-9413
This pr refactors how we render the tab titles for workspace plugins. This makes it so that each plugin is now provides a
headerwhich is connected to their provider and rendered in the workspace.The underlying motivation here is for the data modeling plugin to update it's title based on its store. Down the line I imagine we should have more uses for this. This should reduce coupling between workspaces and plugins and reduce the amount of plugin specific information we have in workspaces.