feat(workspaces): change tab behavior to prevent modified tabs from being closed by accident COMPASS-5022 #5952
Conversation
…ab closing flow; apply initial logic for preventing tabs from closing by accident
| @@ -164,6 +164,21 @@ export function activateWorkspacePlugin( | |||
| } | |||
| }); | |||
|
|
|||
| // TODO(COMPASS-8033): activate this code and account for it in e2e tests and | |||
There was a problem hiding this comment.
Splitting this out to a separate ticket. It works just fine in the browser, but it requires some extra research into how this will affect our e2e tests and generally electron environment where the browser warning is not shown (it just silently doesn't refresh the page)
gribnoysup
added
release notes
and removed
poc
no release notes
gribnoysup
changed the title
| @@ -82,8 +132,7 @@ type CompassWorkspacesProps = { | |||
| onCreateTab(defaultTab?: OpenWorkspaceOptions | null): void; | |||
| onCloseTab(at: number): void; | |||
| onNamespaceNotFound( | |||
| tab: WorkspaceTab, | |||
| connectionId: string, | |||
There was a problem hiding this comment.
Drive-by: no point in having connectionId explicitly in the interface where it's already passed with the tab
| @@ -53,6 +59,7 @@ export const cleanupLocalAppRegistries = () => { | |||
|
|
|||
| export enum WorkspacesActions { | |||
| OpenWorkspace = 'compass-workspaces/OpenWorkspace', | |||
| OpenFallbackWorkspace = 'compass-workspace/OpenFallbackWorkspace', | |||
There was a problem hiding this comment.
Drive-by change: this should've always been a separate action, I was a bit lazy when adding this logic initially
| activeTabId: newActiveTabId, | ||
| tabs: newTabs, | ||
| }; | ||
| return _bulkTabsClose({ |
There was a problem hiding this comment.
Drive-by: at some point this logic was factored out to a separate method, but this place that was the original place for this logic wasn't updated to use it, fixed
| OpenWorkspaceAction | FetchCollectionInfoAction | ||
| > => { | ||
| return ( | ||
| const fetchCollectionInfo = ( |
There was a problem hiding this comment.
Just extracted to a separate thunk action from openWorkspace with no changes
packages/compass-workspaces/src/components/workspace-close-handler.tsx
Outdated
Show resolved
Hide resolved
This is a POC that introduces a new system that allows tabs to control the flow of the closing behavior and additionally changes the tab opening / closing logic to align it more with what VSCode is doing.
New tab flow control interrface allows tabs to register handlers that can return a value telling the workspaces plugin whether or not the tab can be closed or replaced by another tab. In case of workspace communicating that it can't be replaced, new workspace will be opened in the new tab. In case of workspace communicating that it can't be closed, a confirmation modal will pop up on the screen asking to approve the close action.
Using this new interface, this patch also changes the tab opening behavior to the one that is closer to the VSCode behavior with some suggestions from @mcasimir.
isWorkspaceEqualmethod) already exists, move focus to the existing workspaceI recorded some examples of the new behavior, but it's probably better to just pull the branch (or use the build when it's ready) and try it out locally.
Opening new tab over existing one:
Closing Aggregations tab with / without pipeline:
To be very clear: this is not a final behavior we already decided on, we're just trying out things more or less aligned with what design team is currently discussing, any suggestions are welcome and we definitely should define it in text somewhere and ask for confirmation from everyone involved in this discussion.