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 elipses back in to some command names #17715

Merged
merged 6 commits into from
Aug 21, 2024
Merged

Add elipses back in to some command names #17715

merged 6 commits into from
Aug 21, 2024

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Aug 14, 2024

In #16886, the key for the nested action got renamed from Split Pane... to Split pane. This accidentally caused a collision because now there's two actions with the same name! The settings model then prefers the user's action over the one defined in defaults.json, thus completely hiding the nested version.

I tried to balance the stylistic recommendations from #16846 (mainly this comment since it gave some excellent examples) while trying to maintain muscle memory as much as possible (with similar substring sequences). There was also one case where we still used "the tab" so I removed the "the" for consistency.

Side effect of #16886 which closed #16846
Closes #17294, #17684

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Aug 14, 2024
@carlos-zamora carlos-zamora marked this pull request as ready for review August 14, 2024 22:11
@@ -445,7 +445,7 @@
<value>Switch to the last tab</value>
</data>
<data name="TabSearchCommandKey" xml:space="preserve">
<value>Search for tab</value>
<value>Search for tab...</value>
Copy link
Member

Choose a reason for hiding this comment

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

Should this (these?) use … instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

My vote is that Search for tab... should have the ... because the action is requesting more information before it's completed. Invoking the action doesn't search; it opens a new control that then requests for more information (via textbox) to then search for the tab.

Based this decision on generalizing this, but open to discuss and change 😊.

Copy link
Member

@lhecker lhecker Aug 21, 2024

Choose a reason for hiding this comment

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

I meant whether we should use the dedicated ellipsis Unicode character for this: https://www.compart.com/en/unicode/U+2026

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! My stance is "eh". Is there a value to switching over to the unicode character? Screen readers seem to handle it no differently.

@microsoft-github-policy-service microsoft-github-policy-service bot 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 Aug 15, 2024
<value>Open suggestions</value>
<value>Open suggestions...</value>
Copy link
Member Author

Choose a reason for hiding this comment

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

Reverting this one. We use it here so it'd look weird with the .... Besides, we actually are opening the suggestions. We're not necessarily asking for more information.

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'm not gonna block, but also I dunno if I really love not having the ... on nested commands anymore. I miss them. We're not really a traditional nested menu, so the same UX guidelines don't really apply.

But maybe also that's just me being overly attached to nt. as my "pick the profile" shorthand.

@DHowett DHowett enabled auto-merge (squash) August 20, 2024 22:54
@carlos-zamora carlos-zamora added the Needs-Second It's a PR that needs another sign-off label Aug 21, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Second It's a PR that needs another sign-off label Aug 21, 2024
@DHowett DHowett merged commit 0a7c258 into main Aug 21, 2024
20 checks passed
@DHowett DHowett deleted the dev/cazamor/cmd-names branch August 21, 2024 18:13
DHowett pushed a commit that referenced this pull request Aug 21, 2024
In #16886, the key for the nested action got renamed from `Split
Pane...` to `Split pane`. This accidentally caused a collision because
now there's two actions with the same name! The settings model then
prefers the user's action over the one defined in defaults.json, thus
completely hiding the nested version.

I tried to balance the stylistic recommendations from #16846 (mainly
[this
comment](#16846 (comment))
since it gave some excellent examples) while trying to maintain muscle
memory as much as possible (with similar substring sequences). There was
also one case where we still used "the tab" so I removed the "the" for
consistency.

Side effect of #16886 which closed #16846
Closes #17294
Closes #17684

(cherry picked from commit 0a7c258)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSFdXc
Service-Version: 1.21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CmdPal Command Palette issues and features Area-User Interface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
4 participants