Skip to content
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

feat(compass-workspaces): adapt WorkspacePlugin to support tabs with multiple DataServices COMPASS-7718 #5667

Conversation

himanshusinghs
Copy link
Contributor

Description

Checklist

Motivation and Context

  • Bugfix
  • New feature
  • Dependency update
  • Misc

Open Questions

Dependents

Types of changes

  • Backport Needed
  • Patch (non-breaking change which fixes an issue)
  • Minor (non-breaking change which adds functionality)
  • Major (fix or feature that would cause existing functionality to change)

@github-actions github-actions bot added the feat label Apr 8, 2024
@himanshusinghs himanshusinghs added no release notes Fix or feature not for release notes and removed feat labels Apr 8, 2024
@github-actions github-actions bot added the feat label Apr 8, 2024
@paula-stacho
Copy link
Contributor

aren't connectionInfoId and connectionId the same thing?

@himanshusinghs himanshusinghs force-pushed the feat/COMPASS-7718-workspace-adapted-to-support-different-dataservice branch from 938b0af to a6bd695 Compare April 19, 2024 16:37
@himanshusinghs
Copy link
Contributor Author

himanshusinghs commented Apr 19, 2024

aren't connectionInfoId and connectionId the same thing?

yeap they are the same - I renamed just now the instances of connectionInfoId to connectionId in this PR. There are a few more instances of this but those are from old PRs, I will rename them at a later point.

@himanshusinghs himanshusinghs force-pushed the feat/COMPASS-7718-workspace-adapted-to-support-different-dataservice branch 2 times, most recently from 5500fa7 to 044e83f Compare April 22, 2024 09:01
@himanshusinghs
Copy link
Contributor Author

Besides the points I mentioned there is one other point I need to confirm which are the changes for compass-web. In its current state the CompassWeb tab router does not add ConnectionInfo to the returned WorkspaceOptions. There are two ways to approach it:

  • make ConnectionId part of URL path so that it resolves alongside the workspace options but I am not able to fully understand what it would mean for mms
  • silently add ConnectionId to the workspace options but this felt a bit hacky

So excluding this, the PR should be ready for review.

@himanshusinghs himanshusinghs marked this pull request as ready for review April 22, 2024 10:18
@paula-stacho
Copy link
Contributor

paula-stacho commented Apr 22, 2024

@kmruiz do we already have a follow up task to fix the sidebar with the new activeWorkspace.connectionId? If we don't I can create it

@kmruiz
Copy link
Contributor

kmruiz commented Apr 22, 2024

@kmruiz do we already have a follow up task to fix the sidebar with the new activeWorkspace.connectionId? If we don't I can create it

I believe this was part of this task, but if we want to do it in another please feel free to create a ticket and we can play it when we merge this PR.

@himanshusinghs
Copy link
Contributor Author

@paula-stacho All the usage of WorkspaceService already pass connectionId in this PR so that part is covered.

@paula-stacho
Copy link
Contributor

Re @himanshusinghs and @kmruiz

ConnectionId is passed to the new sidebar, but it doesn't trickle all the way down to ConnectionsNavigationTree. So the highlighting and expanding behaviour of connection items isn't complete. We weren't sure what the new structure of Workspace will be, so we didn't prepare that part. I'd say it can easily be a follow up.

Copy link
Contributor

@kmruiz kmruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@kmruiz
Copy link
Contributor

kmruiz commented Apr 22, 2024

Re @himanshusinghs and @kmruiz

ConnectionId is passed to the new sidebar, but it doesn't trickle all the way down to ConnectionsNavigationTree. So the highlighting and expanding behaviour of connection items isn't complete. We weren't sure what the new structure of Workspace will be, so we didn't prepare that part. I'd say it can easily be a follow up.

Then please feel free to create a task, we can play it when we merge this PR. From what you say, when we merge this passing down the connection id should be pretty straightforward.

Copy link
Contributor

@paula-stacho paula-stacho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Changes:
- modifed WorkspacePlugin to not depend on MongoDBInstance and
  DataService anymore
- made connectionId a tab state
- WorkspaceService now asks connectionId in exposed methods
- uses ConnectionInfoProvider in:
  - WorkspacesPlugin to wrap active workspace element
  - SidebarPlugin to wrap single connection sidebar
  - compass/workspaces.tsx to render global modals wrapped with
    ConnectionInfoProvider
- modified places which used useOpenWorkspace hook to also pass the
  connectionId now
@himanshusinghs himanshusinghs force-pushed the feat/COMPASS-7718-workspace-adapted-to-support-different-dataservice branch from 0984985 to a5facf3 Compare April 23, 2024 08:06
@himanshusinghs himanshusinghs merged commit 1d8e50a into main Apr 23, 2024
16 checks passed
@himanshusinghs himanshusinghs deleted the feat/COMPASS-7718-workspace-adapted-to-support-different-dataservice branch April 23, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat no release notes Fix or feature not for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants