-
Notifications
You must be signed in to change notification settings - Fork 229
feat(workspaces): context menu on tabs to duplicate and close all other tabs #7053
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
base: gagik/context-menu-compass-ui
Are you sure you want to change the base?
Conversation
dispatch({ type: WorkspacesActions.CloseTab, atIndex }); | ||
cleanupLocalAppRegistryForTab(tab?.id); | ||
dispatch({ type: WorkspacesActions.CloseAllOtherTabs, atIndex }); | ||
cleanupLocalAppRegistryForTab(tabs[atIndex].id); |
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 want to cleanup for tabs that you closed, not for the only one that stays open. We usually do this through cleanupRemovedTabs
method used in similar cases in this slice
const { tabs } = getState(); | ||
const otherTabs = tabs.filter((_, index) => index !== atIndex); | ||
for (const tab of otherTabs) { | ||
if (!canCloseTab(tab) && !(await confirmClosingTabs())) { |
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.
Doesn't feel right that canceling close for one tab stops all the others from closing too (or the other way around, you say "yes" for one, then "no" for other, and all stay open), in vscode you'd be asked for evey one separately and they are closed based on your answers. Wouldn't that make more sense for our flow too?
Description
Merging this PR will:
tabs-context-menu.mov
Checklist
Motivation and Context
Open Questions
I feel the tooltip is getting in the way of this interaction and while we could solve this in a bespoke way (ex disabling the tooltip component if the context menu
isOpen
), we could disable the underlying UI (using ex thepointer-events
CSS property) when the menu is in its opened state.Dependents
Types of changes