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

Add help text to 'more options' in command palette #9271

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Feb 24, 2021

Similar to #9262. This creates another data template specifically for
command palette items that open up more options. We leverage the
localization key from #9262 to apply help text to this template
automatically.

Using the data template approach, we now have no need for the
HasNestedComandsVisibilityConverter, so that set of files is now
deleted. The logic to detect nested commands was moved to the template
selector.

PR Checklist

Validation Steps Performed

Tested using NVDA.

@ghost ghost added Area-Accessibility Issues related to accessibility Area-CmdPal Command Palette issues and features Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Impact-Compliance It gotta be this way. Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Feb 24, 2021
Comment on lines +70 to +77
<!-- The block for the key chord is only visible
when there's actual text set as the label. See
CommandKeyChordVisibilityConverter for details.
We're setting the accessibility view on the
border and text block to Raw because otherwise,
Narrator will read out the key chord. Problem is,
it already did that because it was the list item's
"AcceleratorKey". It's redundant. -->
Copy link
Member

Choose a reason for hiding this comment

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

I do not believe that you can set a key chord on a setting with nested settings under it -- @zadjii-msft, can you?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, this isn't even the same template. Question stands though!

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want nested commands to have key chords in the future? So that when you press them they just open up a menu directly to the command palette filtered to the subcommands???

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I want, tbh. keybinding --> submenu.

Copy link
Member

Choose a reason for hiding this comment

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

I think that was a "future consideration" for the command palette, but never really thought through too hard

@DHowett
Copy link
Member

DHowett commented Feb 24, 2021

("tested using nvda" - did we test with narrator as well? It's inbox, so we must.)

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 this. One fewer converter, and our datamodel is becoming more clear!

@DHowett DHowett requested a review from zadjii-msft February 24, 2021 18:53
@DHowett DHowett changed the title [main] Add help text to 'more options' in command palette Add help text to 'more options' in command palette Feb 24, 2021
@carlos-zamora
Copy link
Member Author

("tested using nvda" - did we test with narrator as well? It's inbox, so we must.)

Tested with narrator too. It works, but you need to specifically request the help text.
NVDA automatically reads the help text for you.

That's not ideal, but I think it comes down to narrator's "philosophy" vs NVDA's. Injecting "more options" into the name (automation property) is specific guidance we've been told to avoid too.

@DHowett
Copy link
Member

DHowett commented Feb 24, 2021

Huh.

@DHowett DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. Needs-Second It's a PR that needs another sign-off labels Feb 24, 2021
@ghost ghost requested review from miniksa, leonMSFT and PankajBhojwani February 24, 2021 19:18
@DHowett DHowett merged commit d1bf0fc into main Feb 24, 2021
@DHowett DHowett deleted the dev/cazamor/a11y/main-cmd-plt-more-options branch February 24, 2021 20:02
@DHowett DHowett removed the zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. label Feb 24, 2021
DHowett pushed a commit that referenced this pull request Feb 24, 2021
Similar to #9262. This creates another data template specifically for
command palette items that open up more options. We leverage the
localization key from #9262 to apply help text to this template
automatically.

Using the data template approach, we now have no need for the
`HasNestedComandsVisibilityConverter`, so that set of files is now
deleted. The logic to detect nested commands was moved to the template
selector.

## Validation Steps Performed
Tested using NVDA.

Addresses #7908 better

(cherry picked from commit d1bf0fc)
@ghost
Copy link

ghost commented Mar 1, 2021

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

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Accessibility Issues related to accessibility Area-CmdPal Command Palette issues and features Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Impact-Compliance It gotta be this way. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Second It's a PR that needs another sign-off Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants