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

Enable moving back to the root level in the command palette #8051

Merged
5 commits merged into from Nov 5, 2020
Merged

Enable moving back to the root level in the command palette #8051

5 commits merged into from Nov 5, 2020

Conversation

ghost
Copy link

@ghost ghost commented Oct 27, 2020

This commit adds functionality so that users can move back from sub menu
whenever they want. As a result, users no longer have to close command
palette and open it again to get all commands again.

Closes #7910

@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Impact-Compliance It gotta be this way. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 27, 2020
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.

Thanks for throwing this together! I think when I was envisioning this, I was imagining it less as "use backspace to go up a level", but more as an actual button to go up a level. I don't know about other users, but I'm definitely the type of person to just mash the Bksp key when I want to clear out the input, usually far more than is actually needed. I think I pictured something like this:

image

And clicking on the < would take you up the level.

This is the mockup code I had in XAML Studio:

            <StackPanel Orientation="Horizontal"
                    Padding="16, 0, 16, 4"
                    Grid.Row="1"> 
                <FontIcon 
                    FontSize="12"
                    FontFamily="Segoe MDL2 Assets" 
                    Glyph="&#xE76b;">
                
                </FontIcon>
                <TextBlock
                    Padding="16, 0, 16, 4"
                    x:Name="_parentCommandText"
                    FontStyle="Italic"
                    Visibility="Visible"
                    Text="New tab with...">
                </TextBlock>
            </StackPanel>

@ghost ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Oct 27, 2020
@ghost
Copy link
Author

ghost commented Oct 28, 2020

I will fix to just clicking on the <, I included parentCommandName as well.

@github-actions
Copy link

New misspellings found, please review:

  • moveback
To accept these changes, run the following commands
perl -e '
my @expect_files=qw('".github/actions/spell-check/expect/alphabet.txt
.github/actions/spell-check/expect/expect.txt
.github/actions/spell-check/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"akb appdata Autogenerated autologin CParams cppcorecheck debian debugbreak decf DECLL DECSMBV esa FILEPATH guidgenerator hhhh Inplace keith keybound naws notypeopt openlogo restrictederrorinfo rgus richturn Scs SGRXY subnegotiation Switchto telnetpp winver Wlk wslhome xe "');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
  if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
  next if /^($re)(?:$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spell-check/expect/e68e0ddc9d52fbb4c34c1952fa225655c001736b.txt";
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"autogenerated CPPCORECHECK Debian filepath inplace moveback WINVER "');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a) cmp lc($b)} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;'
git add .github/actions/spell-check/expect || echo '... you want to ensure .github/actions/spell-check/expect/e68e0ddc9d52fbb4c34c1952fa225655c001736b.txt is added to your repository...'
✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

  • ... misspelled, then please correct them instead of using the command.
  • ... names, please add them to .github/actions/spell-check/dictionary/names.txt.
  • ... APIs, you can add them to a file in .github/actions/spell-check/dictionary/.
  • ... just things you're using, please add them to an appropriate file in .github/actions/spell-check/expect/.
  • ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spell-check/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The :check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

⚠️ Reviewers

At present, the action that triggered this message will not show its ❌ in this PR unless the branch is within this repository.
Thus, you should make sure that this comment has been addressed before encouraging the merge bot to merge this PR.

@zadjii-msft zadjii-msft self-assigned this Oct 28, 2020
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.

I think I was expecting something more like

            <StackPanel Orientation="Horizontal"
                    Padding="16, 0, 16, 4"
                    Grid.Row="1"
                    Visibility="{x:Bind ParentCommandName,
                                 Mode=OneWay,
                                 Converter={StaticResource ParentCommandVisibilityConverter}}"> 

                <Button Background="Transparent" x:Name="_parentCommandBackButton" x:Uid="ParentCommandBackButton"> 
                    <FontIcon 
                        FontSize="12"
                        FontFamily="Segoe MDL2 Assets" 
                        Glyph="&#xE76b;">
                    </FontIcon>
                </Button>
                
                <TextBlock
                    Padding="16, 0, 16, 4"
                    x:Name="_parentCommandText"
                    FontStyle="Italic"
                    Grid.Row="1"
                    Text="{x:Bind ParentCommandName, Mode=OneWay}">
                </TextBlock>
            </StackPanel>

That way, command palette shouldn't need to futz with editing the text of the parent command to prepend the <, it should just work. GrantedI've only tested this in XAML Studio, so I'm not positive this will work right.

Also - make sure to add

  <data name="ParentCommandBackButton.[using:Windows.UI.Xaml.Automation]AutomationProperties.Name" xml:space="preserve">
    <value>Back</value>
  </data>
  <data name="ParentCommandBackButton.[using:Windows.UI.Xaml.Controls]ToolTipService.ToolTip" xml:space="preserve">
    <value>Back</value>
  </data>

to the Resources.resw file, so the button will have a tooltip and appear in UIA

@@ -1,51 +1,59 @@
<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under
Copy link
Member

Choose a reason for hiding this comment

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

Weird that these xaml files keep getting reformatted by your editor - mind reverting them?

@@ -2,106 +2,158 @@
x:Class="TerminalApp.ColorPickupFlyout"
Copy link
Member

Choose a reason for hiding this comment

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

(revert this one too)

Copy link
Author

Choose a reason for hiding this comment

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

I think it's something to do with clang format, but I don't know how to fix it.

void CommandPalette::_moveBackButtonClicked (Windows::Foundation::IInspectable const& /*sender*/,
Windows::UI::Xaml::RoutedEventArgs const&)
{
_searchBox().Visibility(Visibility::Collapsed);
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you'll need to hide the search box then make it re-appear, right?

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 28, 2020
@zadjii-msft zadjii-msft assigned ghost and unassigned zadjii-msft Oct 28, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 29, 2020
@ghost
Copy link
Author

ghost commented Oct 29, 2020

“Resources.resw file, so the button will have a tooltip and appear in UIA”
I could not fully understand what this mean even though I added to the file.
What is tooltip and UIA?

@zadjii-msft
Copy link
Member

What is tooltip and UIA?

A tooltip is the little helper text that appears over an item when you hover it with the mouse. For example:
image
That "Close" text is the tooltip for the close button.

UIA in this context refers to "UI Automation", which is a set of APIs that allow certain accessibility tools to navigate UIs. This enables screen readers to be able to list the name of the control, so users who are visually impaired can know what the control is

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.

Looks good to me, thanks!

@zadjii-msft
Copy link
Member

(make sure to run tools\runformat.cmd from the root of the repo to make sure the code is formatted correctly)

@zadjii-msft zadjii-msft unassigned ghost Oct 29, 2020
Copy link
Member

@miniksa miniksa 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 to me.

@zadjii-msft
Copy link
Member

@Hegunumo please make sure to run tools\runformat.cmd from the root of the repo to make sure the code is formatted correctly ☺️

@zadjii-msft zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 3, 2020
@ghost
Copy link

ghost commented Nov 3, 2020

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost
Copy link
Author

ghost commented Nov 3, 2020

Do I run it right before submit PR or right after forked the repository ?
Are these 3 failings due to code formatting ?
Do 8 checks always have to be successful in order to get merged ?
By the way, thank you for you helps 😅.

@zadjii-msft
Copy link
Member

You can just run the script locally, then commit & push the changes. The script will auto-format the code for you. This will resolve the "Code Health Scripts Proper Code Formatting Check" check, which will also in turn resolve the "Terminal CI" check.

We're also just gonna get rid of the "Lint Code Base" step, because it is incredibly flaky. So don't worry about that.

@ghost ghost removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 3, 2020
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

I love it. Thank you!

@DHowett DHowett changed the title Enable move back from sub menu Enable moving back up a level in the command palette Nov 5, 2020
@DHowett DHowett changed the title Enable moving back up a level in the command palette Enable moving back to the root level in the command palette Nov 5, 2020
@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Nov 5, 2020
@ghost ghost merged commit 341bb4f into microsoft:main Nov 5, 2020
@DHowett
Copy link
Member

DHowett commented Nov 5, 2020

Eventually we can make it pop the action stack instead of going up to the root, but this is way better than it was 😄

@ghost ghost deleted the dev/7910-move_out_sub branch November 5, 2020 03:33
@ghost
Copy link

ghost commented Nov 11, 2020

🎉Windows Terminal Preview v1.5.3142.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal AutoMerge Marked for automatic merge by the bot when requirements are met hacktoberfest-accepted Impact-Compliance It gotta be this way. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
4 participants