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.

Sign up for GitHub

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

Jump to bottom

Updated pane context menu #18126

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Updated pane context menu #18126

wants to merge 28 commits into from
+161 −18

Conversation

dmitrykok
Copy link

@dmitrykok dmitrykok commented Oct 30, 2024 •
edited
Loading

Motivation

The motivation is that Windows users are more accustomed to working with GUI Menus using a mouse, unlike Linux users.

Summary of the Pull Request

added split pane with profile or duplicate up/down/left/right context menus as submenu
added swap panes up/down/left/right context menus as submenu
added toggle pane zoom context menu
added close other panes context menu

References :

Type of change :

  • New feature

Split right to Ubuntu profile from main pane:

image

Split down duplicate:

image

Split down to CMD profile from Ubuntu pane:

image

Split right to Fedora profile from Ubuntu pane:

image

Result window, with swap menus, you also can see "toggle pane zoom" and "close other panes" menus:

image

@dmitrykok
Copy link
Author

@microsoft-github-policy-service agree

@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@dmitrykok
Update settings.json
a57aff8 
remove added values that fail ckecks
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
zadjii-msft
zadjii-msft requested changes Oct 30, 2024
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

quick feedback before I review the rest of this

src/cascadia/TerminalApp/Resources/de-DE/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 30, 2024
@dmitrykok
put back comments removed by mistake in TerminalPage.cpp, revert the …
4ab861c 
…changes to all the Resources.resw files that aren't in the en-us dir
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 30, 2024
@dmitrykok dmitrykok requested a review from zadjii-msft October 30, 2024 14:29
@dmitrykok
Copy link
Author

quick feedback before I review the rest of this

Hi @zadjii-msft, All requested changes are addressed and fixed.

@dmitrykok dmitrykok mentioned this pull request Nov 1, 2024
Open
@htcfreek
Copy link

htcfreek commented Nov 1, 2024

It feels odd that the split pane entries exists twice on the second level. Can you add a "same profile"/"duplicate" entry at the first position of the third level instead?

@dmitrykok
the split pane entries exists twice on the second level, add a 'dupli…
f682222 
…cate pane' entry at the first position of the third level
@dmitrykok
Copy link
Author

It feels odd that the split pane entries exists twice on the second level. Can you add a "same profile"/"duplicate" entry at the first position of the third level instead?

You're right, it looks better.
Done.

@htcfreek
Copy link

htcfreek commented Nov 7, 2024 •
edited
Loading

@dmitrykok
Maybe it is better visible with a separation line between the "duplicate" entry and the other ones. I think that would promote the duplicate entry more. What do you think?

@dmitrykok
add visible with a separation line between the 'duplicate' entry and …
3bcaa51 
…the other ones, changed from 'Duplicate pane' to 'Duplicate <profile_name>'
@dmitrykok
Copy link
Author

@dmitrykok Maybe it is better visible with a separation line between the "duplicate" entry and the other ones. I think that would promote the duplicate entry more. What do you think?

@htcfreek yes, it looks better, also changed from "Duplicate pane" to "Duplicate <profile_name>".

image

image

@htcfreek
Copy link

htcfreek commented Nov 7, 2024

@dmitrykok
Looks good. But why is the last menu item cut off?

@dmitrykok
Copy link
Author

dmitrykok commented Nov 7, 2024 •
edited
Loading

@dmitrykok Looks good. But why is the last menu item cut off?
@htcfreek
It's not cut off, there is a scroll bar, it is disappearing after a couple of seconds if you're not scrolling and shown back if you begin to scroll.

image

@dmitrykok
Copy link
Author

Updated swap menu.
Show only available options:
image
image

@lhecker lhecker added the Needs-Attention The core contributors need to come back around and look at this ASAP. label Dec 4, 2024
zadjii-msft
zadjii-msft reviewed Dec 5, 2024
View reviewed changes
Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

Sorry for the delay looping back here.

Overall, there's nothing blocking here, just a collection of nits I'll give you the chance to fix. If I go MIA again, treat this as a ✅

src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
dmitrykok
dmitrykok commented Dec 8, 2024
View reviewed changes
Copy link
Author

@dmitrykok dmitrykok left a comment

Choose a reason for hiding this comment

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

All requested changes done.

@htcfreek
Copy link

htcfreek commented Dec 8, 2024

@dmitrykok
Did you miss to push the commits?

@lhecker
Copy link
Member

lhecker commented Dec 9, 2024

We've discussed the UX of the PR today, as seen in your PR description. We felt like we should not have the directions submenu and instead go directly to the profile list. Like this basically:

image

The split mode in this case would be "automatic". This is something we can do before merging the PR or after, but it's something we'd like to do before shipping this PR.

@zadjii-msft zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Attention The core contributors need to come back around and look at this ASAP. labels Dec 11, 2024
@dmitrykok
Copy link
Author

@zadjii-msft don't see what changes are requested, and how to get there

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Dec 11, 2024
@zadjii-msft
Copy link
Member

Yea I marked "needs feedback" because you said "All requested changes done" but forgot to push the commit 😉

@dmitrykok
cache the value of focusedProfile.Name() and focusedProfile.Icon(), c…
9a936f4 
…ache the value of profile.Name() and profile.Icon(), control always show Find by itself
@dmitrykok
No need to manually override the value from the profile, with the val…
8cf4ad6 
…ue from the profile
@dmitrykok
Copy link
Author

dmitrykok commented Dec 11, 2024 •
edited
Loading

Hi @zadjii-msft, all nits fixed 😉.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft Awaiting requested review from zadjii-msft

Requested changes must be addressed to merge this pull request.

Assignees

@dmitrykok dmitrykok

Labels
Needs-Attention The core contributors need to come back around and look at this ASAP.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dmitrykok @htcfreek @lhecker @zadjii-msft