-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add support for a "Move Tab to New Window" tab context menu item #15376
Add support for a "Move Tab to New Window" tab context menu item #15376
Conversation
This comment has been minimized.
This comment has been minimized.
@Jaswir thanks so much for working on this! We are busy preparing for Microsoft's developer conference, so we might be a little slower than usual to respond. Don't worry -- we're really excited to review your code! @zadjii-msft was on vacation until recently, which is why he's been less quick to respond. Somebody is on the repository every day, typically, but it's hard to get our attention on any individual issue sometimes. That's my fault. 😄 |
This comment was marked as resolved.
This comment was marked as resolved.
:O What conference? |
Microsoft Build, which is kinda the biggest conference for us. |
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.
Honestly, this all looks good. Just fix up those typos
<data name="MoveTabtoNewWindowToolTip" xml:space="preserve"> | ||
<value>Moves tab to a new window using the active profile in the current directory</value> |
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.
<data name="MoveTabtoNewWindowToolTip" xml:space="preserve"> | |
<value>Moves tab to a new window using the active profile in the current directory</value> | |
<data name="MoveTabToNewWindowToolTip" xml:space="preserve"> | |
<value>Moves tab to a new window</value> |
- Capitalize the
To
to make the bot happy - I'd suggest removing the "using the active profile in the current directory" - that would seemingly imply that a new tab is created, when really, the current tab is just moving to the new window, connection and all.
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.
I don't get point 2:
"using the active profile in the current directory."
I interpreted this to mean that the current tab information (active profile in current directory) is used for the tab you want to move. And so you don't create a new tab, but move the current tab (using the active profile in the current directory) to the new window.
Can I somehow change the typos inside here? Just choose, accept suggestion or something? |
Accept suggestion ??? Is this possible? Co-authored-by: Mike Griese <migrie@microsoft.com>
Suggestion Accepted?? Co-authored-by: Mike Griese <migrie@microsoft.com>
Accept suggestion Co-authored-by: Mike Griese <migrie@microsoft.com>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@zadjii-msft |
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.
Thanks!
You're welcome |
@zadjii-msft |
Ah I think I fixed it. But you'll need to review it again, sorry. |
Summary of the Pull Request
Add the "Move Tab to New Window" item to the context menu of the tabs
References and Relevant Issues
#15127
Detailed Description of the Pull Request / Additional comments
Add the "Move Tab to New Window" item to the context menu of the tabs.
Validation Steps Performed
Checked Code Style
https://github.com/microsoft/terminal/blob/main/doc/STYLE.md
PR Checklist